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

[Merged by Bors] - auth for engine api #3046

Closed
wants to merge 19 commits into from
Closed

[Merged by Bors] - auth for engine api #3046

wants to merge 19 commits into from

Conversation

pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Feb 28, 2022 •
edited
Loading

Issue Addressed

Resolves #3015

Proposed Changes

Add JWT token based authentication to engine api requests. The jwt secret key is read from the provided file and is used to sign tokens that are used for authenticated communication with the EL node.

  • Interop with geth (synced merge-devnet-4 with the merge-kiln-v2 branch on geth)
  • Interop with other EL clients (nethermind on merge-devnet-4)
  • Implement zeroize for jwt secrets
  • Add auth server tests with mock_execution_layer
  • Get auth working with the execution_engine_integration tests

@pawanjay176 pawanjay176 added the work-in-progress PR is a work-in-progress label Feb 28, 2022
@paulhauner paulhauner added the bellatrix Required to support the Bellatrix Upgrade label Mar 1, 2022
@paulhauner
Copy link
Member

#3040 has done some refactoring of the execution_engine_integration tests and is likely to merge soon. It might be best to rebase this on #3040 so we don't get conflicts :)

After you rebase on #3040 you may need to run the tests differently (see PR notes). I suggest using this command in the root of this repo:

make test-exec-engine

@pawanjay176 pawanjay176 marked this pull request as ready for review March 2, 2022 14:47
@pawanjay176
Copy link
Member Author

Follows the proposal here #3015 (comment) to generate a new secret in a default location if one isn't provided for any EL endpoint.

Will rebase on unstable once #3040 gets merged.

@paulhauner
Copy link
Member

I've been using this for some testing and it's working really nicely!

#3040 has merged now 🎉 I have a branch here, where I took this branch and rebased it onto unstable and resolved some compile/clippy errors: https://github.com/paulhauner/lighthouse/tree/jwt-auth-paul Feel free to use it, if you want ☺️

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Mar 3, 2022
@pawanjay176
Copy link
Member Author

Hey @paulhauner sorry ended up rebasing myself. This is ready for review now.

paulhauner
paulhauner requested changes Mar 7, 2022
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

This has been working great on merge-devnet-5! I just have a few little things and then we're good to go! 🚀

beacon_node/execution_layer/src/engine_api/auth.rs Outdated Show resolved Hide resolved
beacon_node/execution_layer/src/engines.rs Outdated Show resolved Hide resolved
beacon_node/execution_layer/src/lib.rs Outdated Show resolved Hide resolved
beacon_node/execution_layer/src/lib.rs Show resolved Hide resolved
beacon_node/src/cli.rs Outdated Show resolved Hide resolved
beacon_node/execution_layer/src/engines.rs Outdated Show resolved Hide resolved
@paulhauner 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 Mar 7, 2022
@paulhauner paulhauner mentioned this pull request Mar 7, 2022
Closed
paulhauner
paulhauner requested changes Mar 8, 2022
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Almost there! I have two more suggestions 🙏 One is recommended in this review and the other is at #3046 (comment).

beacon_node/src/config.rs Outdated Show resolved Hide resolved
@paulhauner
Copy link
Member

Oh also, it seems that Geth have updated some CLI flags. If we don't fix them here then bors will fail. The fix is nice and simple: 9be6845

This was referenced Mar 8, 2022
Closed
Closed
pawanjay176 and others added 3 commits March 8, 2022 11:26
@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Mar 8, 2022
paulhauner
paulhauner approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looks good!

Let's try an optimistic bors 🚀

bors r+

bors bot pushed a commit that referenced this pull request Mar 8, 2022
@pawanjay176 @paulhauner
auth for engine api (#3046)
381d0ec 
## Issue Addressed

Resolves #3015 

## Proposed Changes

Add JWT token based authentication to engine api requests. The jwt secret key is read from the provided file and is used to sign tokens that are used for authenticated communication with the EL node.

- [x] Interop with geth (synced `merge-devnet-4` with the `merge-kiln-v2` branch on geth)
- [x] Interop with other EL clients (nethermind on `merge-devnet-4`)
- [x] ~Implement `zeroize` for jwt secrets~
- [x] Add auth server tests with `mock_execution_layer`
- [x] Get auth working with the `execution_engine_integration` tests






Co-authored-by: Paul Hauner <paul@paulhauner.com>
@bors
Copy link

bors bot commented Mar 8, 2022

Pull request successfully merged into unstable.

Build succeeded:

@bors bors bot changed the title auth for engine api [Merged by Bors] - auth for engine api Mar 8, 2022
@bors bors bot closed this Mar 8, 2022
@paulhauner paulhauner mentioned this pull request Mar 9, 2022
Closed
@pawanjay176 pawanjay176 deleted the jwt-auth branch March 9, 2022 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulhauner paulhauner paulhauner approved these changes

Assignees
No one assigned
Labels
bellatrix Required to support the Bellatrix Upgrade ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pawanjay176 @paulhauner