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

Redundant rpc permission checks when authentication enabled #1118

Closed
magooster opened this issue Jun 19, 2020 · 2 comments
Closed

Redundant rpc permission checks when authentication enabled #1118

magooster opened this issue Jun 19, 2020 · 2 comments
Assignees
Labels
TeamRevenant GH issues worked on by Revenant Team

Comments

@magooster
Copy link
Contributor

magooster commented Jun 19, 2020

Description

When authentication is enabled Besu will check all possible rpc method permissions for a user when authorizing an rpc request.

Expected behavior: [What you expect to happen]

Authorization check should return once a suitable permission has been identified.

Actual behavior: [What actually happens]

Besu checks every rpc permission

If debug is set get some confusing log entries as well if user has wider permissions e.g. ["*:*"]

2020-06-19 19:42:57.073+01:00 | vert.x-worker-thread-9 | DEBUG | JsonRpcHttpService | JSON-RPC request -> priv_getTransactionReceipt
2020-06-19 19:42:57.073+01:00 | vert.x-worker-thread-9 | DEBUG | JWTUser | User has no permission [priv:getTransactionReceipt]
2020-06-19 19:42:57.073+01:00 | vert.x-worker-thread-9 | DEBUG | JWTUser | User has no permission [priv:*]

If user has valid permissions I don't need to see those...

Frequency: [What percentage of the time does it occur?]

Whenever authentication is enabled

Versions (Add all that apply)

  • Software version: <=1.4.7

Additional Information

See the isPermitted method in AuthenticationUtils.java

Note the order from getPermissions in JsonRpcMethod.java is not in terms of narrowing permissions, actually retruns

"*:*"
"group:method"
"group:*"

@MadelineMurray
Copy link
Contributor

hi @magooster - the PR to fix this has been merged now and will be available in the next release. Are you happy for this to be closed?

@magooster
Copy link
Contributor Author

Sure, thanks for the update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Development

No branches or pull requests

3 participants