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

Wallet gRPC authentication is nonstandard #5927

Closed
AaronFeickert opened this issue Nov 8, 2023 · 1 comment · Fixed by #5936
Closed

Wallet gRPC authentication is nonstandard #5927

AaronFeickert opened this issue Nov 8, 2023 · 1 comment · Fixed by #5936
Assignees

Comments

@AaronFeickert
Copy link
Collaborator

AaronFeickert commented Nov 8, 2023

The handling of wallet and node gRPC authentication is somewhat nonstandard:

  • The server configuration contains plaintext credentials.
  • The user is provided with a PHC string representing the Argon2 hash of the password.
  • The client (miner/proxy) configuration is updated with the PHC string.
  • When authenticating, the client provides the username and PHC string in an HTTP basic authorization header.
  • The server applies Argon2 to its plaintext password as part of verification.

My understanding had been that this was an intended design choice, given the local threat model. However, it appears that this isn't the case. With ongoing work to support TLS connections for gRPC operations, and recent work adding base node gRPC authorization, the design should be updated to a more standard approach.

The usual PBKDF threat model assumes compromise of the server's password store, and wants to limit the ability of the attacker to determine a password. This isn't really the case here. One design approach could simply be a static shared high-entropy token that the server and client both store, and which is simply checked in constant time during authorization. This is unwieldy and less in line with the idea of HTTP authorization, so a standard username/password approach could still be used to keep things simple for the user.

The design could be the following:

  • Plaintext credentials are supplied during gRPC server setup.
  • The password is processed with Argon2, and its PHC string is stored locally along with a username (which could be blank or a default if desired).
  • The client specifies the password in an HTTP header, read either via configuration or environment variable or runtime options.
  • The server verifies the supplied password against its stored PHC string as part of verification.

There are still other considerations and risks with this:

  • If the HTTP connection isn't otherwise protected, a passive attacker can simply sniff the password on the wire.
  • If the client is compromised, an attacker can read the plaintext password from configuration or environment variable or runtime logs.
@AaronFeickert
Copy link
Collaborator Author

This is related to #5809.

@AaronFeickert AaronFeickert self-assigned this Nov 9, 2023
SWvheerden pushed a commit that referenced this issue Nov 14, 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.
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 a pull request may close this issue.

1 participant