[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Remove VC response signing and fix HTTP error handling #5529

Merged
merged 16 commits into from
Jul 16, 2024
Merged

Remove VC response signing and fix HTTP error handling #5529

merged 16 commits into from
Jul 16, 2024

Conversation

eserilev
Copy link
Collaborator
@eserilev eserilev commented Apr 5, 2024 •
edited by dapplion
Loading

Issue Addressed

#4597 and #5423

Proposed Changes

  • Removed VC response signing and related code paths
  • prevent warp backtracking issues by changing our usage of and_then to then

Closes #4597
Closes #5423

@eserilev
and_then to then
4a2bfb7 
remove expect
move convert_rejection to utils
remove signer from vc api
@eserilev
remove key
e969769
@eserilev eserilev added val-client Relates to the validator client binary HTTP-API ready-for-review The code is ready for review labels Apr 5, 2024
@eserilev eserilev 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 Apr 7, 2024
@michaelsproul michaelsproul mentioned this pull request Apr 12, 2024
Closed
@eserilev
Copy link
Collaborator Author
eserilev commented Apr 27, 2024 •
edited
Loading

I've reverted some of my deletions and refactored a few things

Removing VC response signing

I've removed VC response signing which makes VC response error handling much simpler. Everything is now wrapped in a blocking_json_task which also converts warp errors into Responses to prevent weird warp backtrace nonsense

VC Auth header

I've reverted the changes that removed VC auth header logic.

We just need to change the authentication token to an arbitrary byte string, rather than a token derived from the server's signing key.

In ApiSecret::create_or_open

lighthouse/validator_client/src/http_api/api_secret.rs

Lines 45 to 47 in afccdf8

if !pk_path.exists() {
let sk = SecretKey::random(&mut thread_rng());
let pk = PublicKey::from_secret_key(&sk);

This little snippet already randomly generates an auth token. I've removed the following logic

  • Generating the server signing key from the api token
  • Storing/loading the server signing key on/from disk
  • Checking that the server signing key and api token are a valid key pair

I've also ensured that we're still backwards compatible, by loading the api token from disk if it already exists

@eserilev eserilev 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 Apr 27, 2024
michaelsproul
michaelsproul requested changes Apr 29, 2024
View reviewed changes
Copy link
Member
@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looking good! I think we can get this in for v5.2.0 pretty soon

validator_client/src/http_api/api_secret.rs Outdated Show resolved Hide resolved
common/eth2/src/lighthouse_vc/http_client.rs Outdated Show resolved Hide resolved
@michaelsproul 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 Apr 29, 2024
@eserilev eserilev added the ready-for-review The code is ready for review label Apr 30, 2024
@eserilev eserilev removed the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Apr 30, 2024
michaelsproul
michaelsproul approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member
@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks great, let's do it!

validator_client/src/http_api/api_secret.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. backwards-incompat Backwards-incompatible API change v5.3.0 Q3 2024 release with database changes! and removed ready-for-review The code is ready for review labels Jul 16, 2024
@michaelsproul
Copy link
Member

Marking as backwards-incompat because removing the Signature header is technically a breaking change for the ~0 users who care about this 😁

Let's merge

@michaelsproul
Copy link
Member

@mergify queue

@mergify Mergify
Copy link
mergify bot commented Jul 16, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at bf2f0b0

mergify bot added a commit that referenced this pull request Jul 16, 2024
@mergify
Merge of #5529
57e5be3
@mergify mergify bot mentioned this pull request Jul 16, 2024
Closed
5 tasks
@mergify mergify bot merged commit bf2f0b0 into sigp:unstable Jul 16, 2024
28 checks passed
@chong-he chong-he mentioned this pull request Jul 17, 2024
Closed
@jimmygchen jimmygchen mentioned this pull request Jul 17, 2024
Closed
ThreeHrSleep pushed a commit to ThreeHrSleep/lighthouse that referenced this pull request Aug 1, 2024
@eserilev @ThreeHrSleep
Remove VC response signing and fix HTTP error handling (sigp#5529)
14e5265 
* and_then to then
remove expect
move convert_rejection to utils
remove signer from vc api

* remove key

* remove auth header

* revert

* Merge branch 'unstable' of https://github.com/sigp/lighthouse into vc-api-fix

* merge unstable

* revert

* Merge branch 'unstable' of https://github.com/sigp/lighthouse into vc-api-fix

* Merge branch 'unstable' of https://github.com/sigp/lighthouse into vc-api-fix

* refactor blocking json task

* linting

* revert logging

* remove response signing checks in validtor http_api client

* remove notion of public key, prefixes, and simplify token generation

* fmt

* Remove outdated comment on public key
@chong-he chong-he mentioned this pull request Aug 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change HTTP-API ready-for-merge This PR is ready to merge. v5.3.0 Q3 2024 release with database changes! val-client Relates to the validator client binary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eserilev @michaelsproul