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

JWT support for EL engine calls #4980

Merged
merged 16 commits into from
Feb 17, 2022
Merged

Conversation

arhamj
Copy link
Contributor

@arhamj arhamj commented Feb 12, 2022

PR Description

Pending

  • Generating secret key if not present
  • Adding UTs

Others

  1. Went with adding caching of the Jwt token
  2. Retries on failure is already handled

Fixed Issue(s)

fixes #4958

Documentation

  • I thought about documentation and added the documentation label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@ajsutton ajsutton 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. The build is failing because error prone wants you to use Instant rather than Date. I was wondering if that would be possible as well because things like time comparison and adding works better with Instant. I'm just not sure if using Instant makes it too hard to work with the jwt library if it's expecting a Date though. If it does we could suppress the warning.

Copy link
Contributor

@ajsutton ajsutton 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, thanks for your work on this.

@arhamj arhamj changed the title (WIP) JWT support for EL engine calls JWT support for EL engine calls Feb 16, 2022
Avoid adding authentication to kintsugi and kilnv1 specs. Auth only applies to kilnv2 onwards.
Use assertj better in tests.
Improve error messages when the jwt-secret file can't be found.
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. I've pushed up a couple of small changes so that we only add authentication when the kilnv2 ee version is used, and to ensure we get a clear error message if the jwt-secret file can't be read at startup.

Thanks for getting this done, it's really great work.

At this stage I think we're spec compliant and since kilnv2 isn't the default version yet I'm ok with merging it in. The next step will be testing against EL clients that support authentication to ensure it really is compatible and then working through any issue we find.

@ajsutton ajsutton merged commit 6e940de into Consensys:master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Merge] Support authentication for the Engine API
2 participants