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

rpcserver: Fix websocket auth failure. #2877

Merged
merged 1 commit into from
Feb 9, 2022
Merged

rpcserver: Fix websocket auth failure. #2877

merged 1 commit into from
Feb 9, 2022

Conversation

liukun
Copy link
Contributor

@liukun liukun commented Jan 29, 2022

Fix a bug that allow any user/password combinations except the correct one to pass the authentication of websocket and can act as a limited user.

I tried to add a test but the code is too deep in a long function to write a proper test. A more proper way is to reuse the function func (s *Server) checkAuth() in rpcserver.go, but that changed more lines and make the diff not clear enough. If need I can do the changes.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

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

Good find! This fix looks proper to me and is now the same as the logic for the non-websocket case, which is correct. It looks like it was unintentionally introduced in #2509 when changing the check to be constant time.

It also looks like this case can only be hit if an unauthenticated websocket connection is established to begin with since authenticated connections are correctly rejected here if an incorrect user/pass are provided. This is likely why this wasn't noticed previously.

If you would like to add tests, you could consider doing it in a separate commit or PR from this fix if the concern was just that it is a larger diff.

On a minor note, per the code contribution guidelines, you should wrap your commit description to 72 chars and can also remove the PR checklist from the description now.

@liukun
Copy link
Contributor Author

liukun commented Feb 1, 2022

@rstaudt2 Thanks! I refactored all the similar codes to use a newly created function in rpcserver.go and added a test case of that function. Not too big IMO, so I just commit this together. Also wrapped the commit message to 72 chars.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

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

Creating a single function for this with corresponding tests makes sense to me—I left a few comments inline.

internal/rpcserver/rpcserver.go Outdated Show resolved Hide resolved
internal/rpcserver/rpcserver.go Show resolved Hide resolved
internal/rpcserver/rpcserver_test.go Show resolved Hide resolved
Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

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

The updates look good to me. I noted just a couple of other small things inline.

internal/rpcserver/rpcserver.go Show resolved Hide resolved
internal/rpcserver/rpcserver_test.go Show resolved Hide resolved
internal/rpcserver/rpcserver.go Outdated Show resolved Hide resolved
internal/rpcserver/rpcserver.go Outdated Show resolved Hide resolved
internal/rpcserver/rpcserver.go Outdated Show resolved Hide resolved
@davecgh davecgh changed the title internal/rpcserver: fix websocket auth failure. rpcserver: Fix websocket auth failure. Feb 8, 2022
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

The logic all looks correct now. Thanks for the updates. I have a few minor nits left inline and then it should be good for merge.

internal/rpcserver/rpcserver.go Outdated Show resolved Hide resolved
internal/rpcserver/rpcwebsocket.go Outdated Show resolved Hide resolved
internal/rpcserver/rpcwebsocket.go Outdated Show resolved Hide resolved
Fix a bug that allow any user/password combinations except the correct
one to pass the authentication of websocket as a limited user.
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Updates look good and I've tested the following scenarios:

  • Connecting via HTTP without an auth header results in an authentication failure and immediate disconnection as expected
  • Connecting via HTTP with an auth header that has incorrect credentials results in an authentication failure and immediate disconnection as expected
  • Connecting via HTTP with an auth header that has correct credentials connects as expected
  • Connecting via WebSockets without an auth header is allowed:
    • The authenticate RPC with invalid credentials results in an authentication failure and immediate disconnection as expected
    • The authenticate RPC with valid credentials results in a usable connection as expected
    • Any other RPCs aside from authenticate when not yet authenticated result in immediate disconnection as expected
  • Connecting via WebSockets with an auth header that has incorrect credentials results in an authentication failure and immediate disconnection as expected
  • Connecting via WebSockets with an auth header that has correct credentials connects as expected

Copy link
Member

@rstaudt2 rstaudt2 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 to me with the latest updates.

The test coverage looks good as well:

go test -tags rpctest -run="TestCheckAuthUserPass|TestCheckAuth" -coverprofile=cov.out &&
go tool cover -func=cov.out | grep -E "checkAuthUserPass|checkAuthMAC"

github.com/decred/dcrd/internal/rpcserver/rpcserver.go:5858:  checkAuthMAC  100.0%
github.com/decred/dcrd/internal/rpcserver/rpcserver.go:5879:  checkAuthUserPass  100.0%

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.

4 participants