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

feat(rpc): creating scaffolding for JWT authentication #1325

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Nov 4, 2022

This PR introduces the necessary scaffolding for adding authentication to the OpenRPC server.
This means:

Internal structs

The API struct of every module now stores the functions in an Internal struct. The API structs will now also implement the Module interface, where every method calls the function in it's internal struct. This is ugly, but is necessary in order to use auth with the jsonrpc library for now. We will find a more elegant solution once we have time to implement something custom. The reasoning for this being necessary is complex but I will try to document it a little bit here:

  1. Because the library is based on reflect, if it adds functionality to a method (for example auth), it needs to be able to actually set a field on a struct. This is why the API structs have functions as fields in general. This applies to the client wrapper as well.
  2. These API structs need to also implement the Module interface, because the methods that are added to the server upon registration are the ones that are found via reflection.
  3. Because the API struct both needs the fields and the implementation that calls the fields, we need to store the fields in an Internal struct to avoid the naming collision.

Auth tags

All methods must have an auth tag on their API.Internal field. Right now the available perms are read, write, and admin. No method can be missing a tag, or it will fail to build.

A middleware stub has been added to verify JWTs from the authorization header. This middleware is only used if the header is sent, and otherwise the given permission will default to read.

Default Implementations

We no longer point to default implementations for docgen. The reason we couldn't do this before was because the API structs did not implement their Module interfaces, so the methods could not be discovered on them through reflection.

P2P

I had to generate the mocks for P2P and add the module in a few places. It wouldn't make much sense to do it in a different PR, but if that is preferred, it should be after this one.

Testing

The rpc tests have been updated to ensure that the API structs implement the full API.
Unit tests also fail if a method does not

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2022

Codecov Report

Merging #1325 (22e9b24) into main (95907f4) will decrease coverage by 0.19%.
The diff coverage is 10.29%.

@@            Coverage Diff             @@
##             main    #1325      +/-   ##
==========================================
- Coverage   55.33%   55.14%   -0.20%     
==========================================
  Files         180      194      +14     
  Lines       10962    11800     +838     
==========================================
+ Hits         6066     6507     +441     
- Misses       4296     4657     +361     
- Partials      600      636      +36     
Impacted Files Coverage Δ
api/gateway/availability.go 0.00% <0.00%> (ø)
api/gateway/middleware.go 42.30% <0.00%> (ø)
nodebuilder/das/das.go 0.00% <0.00%> (ø)
nodebuilder/fraud/fraud.go 0.00% <0.00%> (ø)
nodebuilder/header/header.go 0.00% <0.00%> (ø)
nodebuilder/header/service.go 57.14% <0.00%> (ø)
nodebuilder/p2p/p2p.go 51.11% <0.00%> (-34.08%) ⬇️
nodebuilder/share/share.go 0.00% <0.00%> (ø)
share/availability/cache/availability.go 87.50% <0.00%> (ø)
share/availability/full/availability.go 82.35% <0.00%> (ø)
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@distractedm1nd distractedm1nd marked this pull request as ready for review November 25, 2022 09:53
nodebuilder/p2p/p2p.go Show resolved Hide resolved
nodebuilder/p2p/p2p.go Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Very nice description! LGTM.
Requesting changes due to privacy concerns of two API methods.

nodebuilder/state/state.go Show resolved Hide resolved
nodebuilder/das/das.go Outdated Show resolved Hide resolved
nodebuilder/state/state.go Show resolved Hide resolved
nodebuilder/state/state.go Show resolved Hide resolved
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Oh it was hard to understand the problem with reflection. Nice and clever workaround for the given problem! Make it generated and noone will ever read it :)

api/rpc/server.go Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM. Some of the public could go into read instead, but we can revisit those once we revisit what each module should expose and ADR API compliance

Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@distractedm1nd distractedm1nd merged commit 5080aba into celestiaorg:main Dec 6, 2022
@Wondertan Wondertan deleted the rpc-authorization branch December 6, 2022 13:06
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rpc kind:feat Attached to feature PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants