[Merged by Bors] - Implement standard keystore API #2736

michaelsproul · 2021-10-20T03:44:44Z

Issue Addressed

Implements the standard key manager API from https://ethereum.github.io/keymanager-APIs/, formerly ethereum/beacon-APIs#151
Related to #2557

Proposed Changes

Add all of the new endpoints from the standard API: GET, POST and DELETE.
Add a validators.enabled column to the slashing protection database to support atomic disable + export.
Add tests for all the common sequential accesses of the API
Add tests for interactions with remote signer validators
Add end-to-end tests for migration of validators from one VC to another
Implement the authentication scheme from the standard (token bearer auth)

Additional Info

The enabled column in the validators SQL database is necessary to prevent a race condition when exporting slashing protection data. Without the slashing protection database having a way of knowing that a key has been disabled, a concurrent request to sign a message could insert a new record into the database. The delete_concurrent_with_signing test exercises this code path, and was indeed failing before the enabled column was added.

The validator client authentication has been modified from basic auth to bearer auth, with basic auth preserved for backwards compatibility.

michaelsproul · 2021-10-27T01:59:50Z

I've added a draft implementation of the auth scheme described here: ethereum/beacon-APIs#151 (comment)

There's a lot of discussion around the standard, so I'm going to step back from the impl until the dust settles a bit

michaelsproul · 2021-11-10T05:41:37Z

This is almost ready for review!

There's one FIXME remaining about returning the correct error status for the import/delete of web3signer validators.

Other than that I'll be working on getting the API spec into the new repo: https://github.com/ethereum/keymanager-APIs, although I think we could merge something to unstable before that is completely finalised.

We are currently divergent on the eth/v1/auth endpoint, so I'll also move that to a lighthouse prefix.

michaelsproul · 2021-11-12T06:03:10Z

This is finally ready for review! 🎉

CI should pass now that a Windows-specific issue has been addressed in 21037cf.

paulhauner · 2021-11-22T06:42:15Z

I'm starting to review this! There's a conflict, FYI 🙂

paulhauner

This is great, really neat and well tested 👌 I don't have anything substantial to say, I just had a few comments whilst reading it.

It's great to see the standard API progressing 🎉

paulhauner · 2022-01-27T02:40:37Z

common/eth2/src/lighthouse_vc/std_types.rs

+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)]
+#[serde(rename_all = "kebab-case")]


Suggested change

#[serde(rename_all = "kebab-case")]

I'm not sure, but it seems like we don't need this.

Ah, this was just intended to lowercase the variants, so I've switched to rename_all = "lowercase" in 294d44b

paulhauner · 2022-01-28T02:24:32Z

validator_client/src/http_api/keystores.rs

+        .find(|def| def.voting_public_key == pubkey)
+    {
+        if !def.signing_definition.is_local_keystore() {
+            return Err("cannot import duplicate of existing remote signer validator".into());


I notice that an enabled = false validator might not appear in /list but they'll trigger this duplicate warning here. I don't see this as a significant issue and I assume there's going to be some edge-cases when meshing the standard API with our validator management.

Yeah good point. I'm OK with the current behaviour, but yeah, might make sense to re-assess in future.

paulhauner · 2022-01-28T02:35:00Z

validator_client/src/initialized_validators.rs

        let mut definitions_map = HashMap::new();
-        for def in self.definitions.as_slice() {
+        for def in self.definitions.as_slice().iter().filter(|def| def.enabled) {


Hmm, it's interesting that we didn't previously filter enabled = false 🤔 I think it's definitely the right thing to do.

michaelsproul · 2022-01-28T03:40:22Z

Thanks for the review @paulhauner! Will be great to get this out into a release

I think I've addressed the review comments so this should be good to go 🤞

paulhauner

🚀

michaelsproul · 2022-01-30T23:21:46Z

bors r+

## Issue Addressed Implements the standard key manager API from https://ethereum.github.io/keymanager-APIs/, formerly ethereum/beacon-APIs#151 Related to #2557 ## Proposed Changes - [x] Add all of the new endpoints from the standard API: GET, POST and DELETE. - [x] Add a `validators.enabled` column to the slashing protection database to support atomic disable + export. - [x] Add tests for all the common sequential accesses of the API - [x] Add tests for interactions with remote signer validators - [x] Add end-to-end tests for migration of validators from one VC to another - [x] Implement the authentication scheme from the standard (token bearer auth) ## Additional Info The `enabled` column in the validators SQL database is necessary to prevent a race condition when exporting slashing protection data. Without the slashing protection database having a way of knowing that a key has been disabled, a concurrent request to sign a message could insert a new record into the database. The `delete_concurrent_with_signing` test exercises this code path, and was indeed failing before the `enabled` column was added. The validator client authentication has been modified from basic auth to bearer auth, with basic auth preserved for backwards compatibility.

bors · 2022-01-31T01:12:41Z

Pull request successfully merged into unstable.

Build succeeded:

michaelsproul added the work-in-progress PR is a work-in-progress label Oct 20, 2021

michaelsproul force-pushed the std-keymanager-api branch 3 times, most recently from 33d8645 to 3a1b030 Compare October 20, 2021 06:06

michaelsproul mentioned this pull request Oct 20, 2021

Add local keymanager API ethereum/beacon-APIs#151

Closed

michaelsproul added 4 commits November 5, 2021 16:54

Implement standard keystore API

53207a4

Add a re-registration test

f4f2e35

Implement draft /auth API

d0259b4

Update to match latest upstream spec

44e2df8

michaelsproul force-pushed the std-keymanager-api branch from 79b6835 to 44e2df8 Compare November 5, 2021 06:34

michaelsproul added 2 commits November 9, 2021 17:15

Distinguish not_active from not_found during delete

cd30d93

More tests, fix web3signer VC API client

7bf70d5

michaelsproul force-pushed the std-keymanager-api branch from 19a9540 to 7bf70d5 Compare November 9, 2021 06:16

Migration tests, key cache fixes

b7ec7b6

michaelsproul added 5 commits November 11, 2021 14:14

Stringify keystores and slashing protection

31862fe

Fix perf of delete_concurrent_with_signing

29489a3

Fix and test more web3signer interactions

fa62e0a

Move auth API to /lighthouse/auth

98bdeaa

Drop lockfile before delete

21037cf

michaelsproul force-pushed the std-keymanager-api branch from fe46936 to 21037cf Compare November 12, 2021 06:00

michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 12, 2021

michaelsproul requested a review from paulhauner November 12, 2021 06:03

michaelsproul mentioned this pull request Nov 25, 2021

Add implementation details for DELETE ethereum/keymanager-APIs#14

Merged

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Dec 16, 2021

michaelsproul added 4 commits January 11, 2022 09:01

Merge remote-tracking branch 'origin/unstable' into std-keymanager-api

8ee5b66

Merge remote-tracking branch 'origin/unstable' into std-keymanager-api

a4a7fe5

Don't 400 for invalid slashing protection data

0109459

Deny unknown fields in requests

dc3fc28

michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 14, 2022

paulhauner approved these changes Jan 28, 2022

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 28, 2022

michaelsproul added 2 commits January 28, 2022 14:36

Clarify ImportKeystoreStatus attribute

294d44b

Merge remote-tracking branch 'origin/unstable' into std-keymanager-api

17e6445

michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 28, 2022

paulhauner approved these changes Jan 30, 2022

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 30, 2022

bors bot changed the title ~~Implement standard keystore API~~ [Merged by Bors] - Implement standard keystore API Jan 31, 2022

bors bot closed this Jan 31, 2022

michaelsproul deleted the std-keymanager-api branch January 31, 2022 02:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Implement standard keystore API #2736

[Merged by Bors] - Implement standard keystore API #2736

michaelsproul commented Oct 20, 2021 •

edited

Loading

michaelsproul commented Oct 27, 2021

michaelsproul commented Nov 10, 2021

michaelsproul commented Nov 12, 2021

paulhauner commented Nov 22, 2021 •

edited

Loading

paulhauner left a comment

paulhauner Jan 27, 2022

michaelsproul Jan 28, 2022

paulhauner Jan 28, 2022

michaelsproul Jan 28, 2022

paulhauner Jan 28, 2022

michaelsproul commented Jan 28, 2022

paulhauner left a comment

michaelsproul commented Jan 30, 2022

bors bot commented Jan 31, 2022

[Merged by Bors] - Implement standard keystore API #2736

[Merged by Bors] - Implement standard keystore API #2736

Conversation

michaelsproul commented Oct 20, 2021 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

michaelsproul commented Oct 27, 2021

michaelsproul commented Nov 10, 2021

michaelsproul commented Nov 12, 2021

paulhauner commented Nov 22, 2021 • edited Loading

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner Jan 27, 2022

Choose a reason for hiding this comment

michaelsproul Jan 28, 2022

Choose a reason for hiding this comment

paulhauner Jan 28, 2022

Choose a reason for hiding this comment

michaelsproul Jan 28, 2022

Choose a reason for hiding this comment

paulhauner Jan 28, 2022

Choose a reason for hiding this comment

michaelsproul commented Jan 28, 2022

paulhauner left a comment

Choose a reason for hiding this comment

🚀

michaelsproul commented Jan 30, 2022

bors bot commented Jan 31, 2022

michaelsproul commented Oct 20, 2021 •

edited

Loading

paulhauner commented Nov 22, 2021 •

edited

Loading