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

fix!: standardize gRPC authentication and mitigate DoS #5936

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Nov 9, 2023

Description

Standardizes gRPC authentication by removing PHC strings from configuration and preventing a client DoS.

Closes #5809. Closes #5927.

Motivation and Context

As noted in #5927, gRPC authentication is nonstandard. In the current design, server credentials are processed against a client-supplied username and PHC string for verification. This can lead to server DoS, as described in #5809.

This PR fixes the DoS vector. When the gRPC server is started, it applies Argon2 to produce a PHC string that is kept in memory. When a client supplies its (non-PHC) passphrase, it is processed against the stored PHC string. This ensures that the server completely controls the Argon2 parameters that are used.

Note that this is still suboptimal, as the client and server passphrases are still stored in plaintext configuration. Deciding how to handle those is out of scope for this work.

How Has This Been Tested?

It should be tested manually.

What process can a PR reviewer use to test or verify this change?

It should be tested manually.

BREAKING CHANGE: Updates the public APIs for BasicAuthCredentials and ServerAuthenticationInterceptor to accommodate the new behavior. Additionally, existing configuration client/server credentials will stop working, and client credentials will need to use plaintext passwords.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Nov 9, 2023
Copy link

github-actions bot commented Nov 9, 2023

Test Results (CI)

1 258 tests   1 258 ✔️  11m 32s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit b519163.

♻️ This comment has been updated with latest results.

@AaronFeickert AaronFeickert force-pushed the standardize-grpc-auth branch 2 times, most recently from 871b58d to ea5c9f9 Compare November 9, 2023 19:26
Copy link

github-actions bot commented Nov 9, 2023

Test Results (Integration tests)

  2 files  11 suites   15m 3s ⏱️
31 tests 30 ✔️ 0 💤 1
32 runs  31 ✔️ 0 💤 1

For more details on these failures, see this check.

Results for commit b519163.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

I like this approach, looks good. What about adding some unit tests to ServerAuthenticationInterceptor?

@AaronFeickert
Copy link
Collaborator Author

What about adding some unit tests

I looked into this, but it wasn't clear how to cleanly do such tests. Right now, it seems that all of the integration tests use GrpcAuthentication::None and are all part of much larger wallet test suites. Any insight on a simpler approach that would let us test all GrpcAuthentication modes without the extra baggage of the wallet?

@AaronFeickert AaronFeickert marked this pull request as ready for review November 13, 2023 20:48
@AaronFeickert AaronFeickert changed the title fix: standardize gRPC authentication fix: standardize gRPC authentication and mitigate DoS Nov 13, 2023
@AaronFeickert AaronFeickert changed the title fix: standardize gRPC authentication and mitigate DoS fix!: standardize gRPC authentication and mitigate DoS Nov 13, 2023
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 14, 2023
Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

Although I understand the reason for the move it feels a little weird to keep the password in cleartext, even though keeping the hashed password previously didn't change the level of security if someone could access it.
And we are now moving to remove grpc from the wallet entirely so it looks like we're closing down vectors from multiple angles. So I'm good with this.
utAck.

@SWvheerden SWvheerden merged commit 623f127 into tari-project:development Nov 14, 2023
15 of 17 checks passed
@AaronFeickert
Copy link
Collaborator Author

@brianp: I agree that it's not an ideal solution, but I think it meets a reasonable threat model and, improves the user experience, and removes the DoS vector. Even with removing wallet gRPC by default, that attack vector would still exist with node gRPC. Supporting TLS will be an additional layer of protection too. So I think overall it's a net win that doesn't sacrifice practical security.

@AaronFeickert AaronFeickert deleted the standardize-grpc-auth branch November 14, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wallet gRPC authentication is nonstandard Attackers can crash the wallet via the gRPC service
5 participants