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

Remove VC response signing and fix HTTP error handling #5529

Merged
merged 16 commits into from
Jul 16, 2024

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented Apr 5, 2024

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

remove expect
move convert_rejection to utils
remove signer from vc api
@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
@eserilev
Copy link
Collaborator Author

eserilev commented Apr 27, 2024

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

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
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
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

Copy link

mergify bot commented Jul 16, 2024

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 mergify bot merged commit bf2f0b0 into sigp:unstable Jul 16, 2024
28 checks passed
ThreeHrSleep pushed a commit to ThreeHrSleep/lighthouse that referenced this pull request Aug 1, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

2 participants