Skip to content

Commit

Permalink
fix!: standardize gRPC authentication and mitigate DoS (#5936)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@AaronFeickert
AaronFeickert authored Nov 14, 2023
1 parent e2e278c commit 623f127
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 191 deletions.
107 changes: 45 additions & 62 deletions applications/minotari_app_grpc/src/authentication/basic_auth.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,9 @@ use tari_utilities::{ByteArray, SafePassword};
use tonic::metadata::{errors::InvalidMetadataValue, Ascii, MetadataValue};
use zeroize::{Zeroize, Zeroizing};

use crate::authentication::salted_password::create_salted_hashed_password;

const MAX_USERNAME_LEN: usize = 256;

/// Implements [RFC 2617](https://www.ietf.org/rfc/rfc2617.txt#:~:text=The%20%22basic%22%20authentication%20scheme%20is,other%20realms%20on%20that%20server.)
/// Represents the username and password contained within a Authenticate header.
/// Implements [RFC 2617](https://www.ietf.org/rfc/rfc2617.txt) by allowing authentication of provided credentials
#[derive(Debug)]
pub struct BasicAuthCredentials {
/// The username bytes length
Expand Down Expand Up @@ -82,42 +79,29 @@ impl BasicAuthCredentials {
})
}

/// Creates a `Credentials` instance from a base64 `String`
/// which must encode user credentials as `username:password`
pub fn decode(auth_header_value: &str) -> Result<Self, BasicAuthError> {
let decoded = base64::decode(auth_header_value)?;
let as_utf8 = Zeroizing::new(String::from_utf8(decoded)?);

if let Some((user_name, password)) = as_utf8.split_once(':') {
let hashed_password = create_salted_hashed_password(password.as_bytes())?;
let credentials = Self::new(user_name.into(), hashed_password.to_string().into())?;
return Ok(credentials);
}

Err(BasicAuthError::InvalidAuthorizationHeader)
}

/// Creates a `Credentials` instance from an HTTP Authorization header
/// which schema is a valid `Basic` HTTP Authorization Schema.
pub fn from_header(auth_header: &str) -> Result<BasicAuthCredentials, BasicAuthError> {
// check if its a valid basic auth header
/// Parses the contents of an HTTP Authorization (Basic) header into a username and password
/// These can be used later to validate against `BasicAuthCredentials`
/// The input must be of the form `Basic base64(username:password)`
pub fn parse_header(auth_header: &str) -> Result<(String, SafePassword), BasicAuthError> {
// Check that the authentication type is `Basic`
let (auth_type, encoded_credentials) = auth_header
.split_once(' ')
.ok_or(BasicAuthError::InvalidAuthorizationHeader)?;

if encoded_credentials.contains(' ') {
// Invalid authorization token received
return Err(BasicAuthError::InvalidAuthorizationHeader);
}

// Check the provided authorization header
// to be a "Basic" authorization header
if auth_type.to_lowercase() != "basic" {
return Err(BasicAuthError::InvalidScheme(auth_type.to_string()));
}

let credentials = BasicAuthCredentials::decode(encoded_credentials)?;
Ok(credentials)
// Decode the credentials using base64
let decoded = base64::decode(encoded_credentials)?;
let as_utf8 = Zeroizing::new(String::from_utf8(decoded)?);

// Parse the username and password, which must be separated by a colon
if let Some((user_name, password)) = as_utf8.split_once(':') {
return Ok((user_name.into(), password.into()));
}

Err(BasicAuthError::InvalidAuthorizationHeader)
}

// This function provides a constant time comparison of the given username with the registered username.
Expand Down Expand Up @@ -150,7 +134,7 @@ impl BasicAuthCredentials {
/// do the same amount of work irrespective if the username or password is correct or not. This is to prevent timing
/// attacks. Also, no distinction is made between a non-existent username or an incorrect password in the error
/// that is returned.
pub fn constant_time_validate(&self, username: &str, password: &[u8]) -> Result<(), BasicAuthError> {
pub fn constant_time_validate(&self, username: &str, password: &SafePassword) -> Result<(), BasicAuthError> {
let valid_username = self.constant_time_verify_username(username);

// These bytes can leak if the password is not utf-8, but since argon encoding is utf-8 the given
Expand All @@ -159,7 +143,9 @@ impl BasicAuthCredentials {
let str_password = Zeroizing::new(String::from_utf8(bytes)?);
let header_password = PasswordHash::parse(&str_password, Encoding::B64)?;
let valid_password = Choice::from(u8::from(
Argon2::default().verify_password(password, &header_password).is_ok(),
Argon2::default()
.verify_password(password.reveal(), &header_password)
.is_ok(),
));

// The use of `Choice` logic here is by design to hide the boolean logic from compiler optimizations.
Expand Down Expand Up @@ -218,26 +204,20 @@ mod tests {

#[test]
fn it_decodes_from_well_formed_header() {
let credentials = BasicAuthCredentials::from_header("Basic YWRtaW46c2VjcmV0").unwrap();
assert_eq!(credentials.user_name_bytes_length, "admin".as_bytes().len());
let bytes = "admin".as_bytes();
let mut user_name_bytes = [0u8; MAX_USERNAME_LEN];
user_name_bytes[0..bytes.len()].clone_from_slice(bytes);
user_name_bytes[bytes.len()..MAX_USERNAME_LEN]
.clone_from_slice(&credentials.random_bytes[bytes.len()..MAX_USERNAME_LEN]);
assert_eq!(credentials.user_name_bytes, user_name_bytes);
assert!(credentials.constant_time_validate("admin", b"secret").is_ok());
let (username, password) = BasicAuthCredentials::parse_header("Basic YWRtaW46c2VjcmV0").unwrap();
assert_eq!(username, "admin".to_string());
assert_eq!(password.reveal(), b"secret");
}

#[test]
fn it_rejects_header_without_basic_scheme() {
let err = BasicAuthCredentials::from_header(" YWRtaW46c2VjcmV0").unwrap_err();
let err = BasicAuthCredentials::parse_header(" YWRtaW46c2VjcmV0").unwrap_err();
if let BasicAuthError::InvalidScheme(s) = err {
assert_eq!(s, "");
} else {
panic!("Unexpected error: {:?}", err);
};
let err = BasicAuthCredentials::from_header("Cookie YWRtaW46c2VjcmV0").unwrap_err();
let err = BasicAuthCredentials::parse_header("Cookie YWRtaW46c2VjcmV0").unwrap_err();
if let BasicAuthError::InvalidScheme(s) = err {
assert_eq!(s, "Cookie");
} else {
Expand All @@ -264,10 +244,14 @@ mod tests {
// Typical username
let credentials =
BasicAuthCredentials::new("admin".to_string(), hashed_password.to_string().into()).unwrap();
credentials.constant_time_validate("admin", b"secret").unwrap();
credentials
.constant_time_validate("admin", &SafePassword::from("secret".to_string()))
.unwrap();
// Empty username is also fine
let credentials = BasicAuthCredentials::new("".to_string(), hashed_password.to_string().into()).unwrap();
credentials.constant_time_validate("", b"secret").unwrap();
credentials
.constant_time_validate("", &SafePassword::from("secret".to_string()))
.unwrap();
}

#[test]
Expand All @@ -286,16 +270,20 @@ mod tests {
BasicAuthCredentials::new("admin".to_string(), hashed_password.to_string().into()).unwrap();

// Wrong password
let err = credentials.constant_time_validate("admin", b"bruteforce").unwrap_err();
let err = credentials
.constant_time_validate("admin", &SafePassword::from("bruteforce".to_string()))
.unwrap_err();
assert!(matches!(err, BasicAuthError::InvalidUsernameOrPassword));

// Wrong username
let err = credentials.constant_time_validate("wrong_user", b"secret").unwrap_err();
let err = credentials
.constant_time_validate("wrong_user", &SafePassword::from("secret".to_string()))
.unwrap_err();
assert!(matches!(err, BasicAuthError::InvalidUsernameOrPassword));

// Wrong username and password
let err = credentials
.constant_time_validate("wrong_user", b"bruteforce")
.constant_time_validate("wrong_user", &SafePassword::from("bruteforce".to_string()))
.unwrap_err();
assert!(matches!(err, BasicAuthError::InvalidUsernameOrPassword));
}
Expand Down Expand Up @@ -561,21 +549,22 @@ mod tests {

let start = Instant::now();
for short in &short_usernames {
let res = credentials.constant_time_validate(short, b"bruteforce");
let res = credentials.constant_time_validate(short, &SafePassword::from("bruteforce".to_string()));
assert!(res.is_err());
}
let time_taken_1 = start.elapsed().as_millis();

let start = Instant::now();
for long in &long_usernames {
let res = credentials.constant_time_validate(long, b"bruteforce");
let res = credentials.constant_time_validate(long, &SafePassword::from("bruteforce".to_string()));
assert!(res.is_err());
}
let time_taken_2 = start.elapsed().as_millis();

let start = Instant::now();
for _ in 0..COUNTS {
let res = credentials.constant_time_validate(username_actual, b"secret");
let res =
credentials.constant_time_validate(username_actual, &SafePassword::from("secret".to_string()));
assert!(res.is_ok());
}
let time_taken_3 = start.elapsed().as_millis();
Expand Down Expand Up @@ -624,15 +613,9 @@ mod tests {
#[test]
fn it_generates_a_valid_header() {
let header = BasicAuthCredentials::generate_header("admin", b"secret").unwrap();
let cred = BasicAuthCredentials::from_header(header.to_str().unwrap()).unwrap();
assert_eq!(cred.user_name_bytes_length, "admin".as_bytes().len());
let bytes = "admin".as_bytes();
let mut user_name_bytes = [0u8; MAX_USERNAME_LEN];
user_name_bytes[0..bytes.len()].clone_from_slice(bytes);
user_name_bytes[bytes.len()..MAX_USERNAME_LEN]
.clone_from_slice(&cred.random_bytes[bytes.len()..MAX_USERNAME_LEN]);
assert_eq!(cred.user_name_bytes, user_name_bytes);
assert!(cred.constant_time_validate("admin", b"secret").is_ok());
let (username, password) = BasicAuthCredentials::parse_header(header.to_str().unwrap()).unwrap();
assert_eq!(username, "admin".to_string());
assert_eq!(password.reveal(), b"secret");
}
}
}
40 changes: 29 additions & 11 deletions applications/minotari_app_grpc/src/authentication/server_interceptor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,35 +22,53 @@

use log::*;
use tari_common_types::grpc_authentication::GrpcAuthentication;
use tari_utilities::SafePassword;
use tonic::{codegen::http::header::AUTHORIZATION, service::Interceptor, Request, Status};

use crate::authentication::BasicAuthCredentials;
use crate::authentication::{salted_password::create_salted_hashed_password, BasicAuthCredentials};

const LOG_TARGET: &str = "applications::minotari_app_grpc::authentication";

#[derive(Debug)]
pub struct ServerAuthenticationInterceptor {
auth: GrpcAuthentication,
auth: GrpcAuthentication, // this contains a hashed PHC password in the case of basic authentication
}

impl ServerAuthenticationInterceptor {
pub fn new(auth: GrpcAuthentication) -> Self {
Self { auth }
pub fn new(auth: GrpcAuthentication) -> Option<Self> {
// The server password needs to be hashed for later client verification
let processed_auth = match auth {
GrpcAuthentication::None => auth,
GrpcAuthentication::Basic { username, password } => GrpcAuthentication::Basic {
username,
password: create_salted_hashed_password(password.reveal()).ok()?.as_str().into(),
},
};

Some(Self { auth: processed_auth })
}

fn handle_basic_auth(
&self,
req: Request<()>,
valid_username: &str,
valid_password: &[u8],
valid_phc_password: &SafePassword,
) -> Result<Request<()>, Status> {
match req.metadata().get(AUTHORIZATION.as_str()) {
Some(t) => {
let val = t.to_str().map_err(unauthenticated)?;
let credentials = BasicAuthCredentials::from_header(val).map_err(unauthenticated)?;
credentials
.constant_time_validate(valid_username, valid_password)
// Parse the provided header
let header = t.to_str().map_err(unauthenticated)?;
let (header_username, header_password) =
BasicAuthCredentials::parse_header(header).map_err(unauthenticated)?;

// Validate the header credentials against the valid credentials
let valid_credentials =
BasicAuthCredentials::new(valid_username.to_owned(), valid_phc_password.clone())
.map_err(unauthenticated)?;
valid_credentials
.constant_time_validate(&header_username, &header_password)
.map_err(unauthenticated)?;

Ok(req)
},
_ => Err(unauthenticated("Missing authorization header")),
Expand All @@ -64,8 +82,8 @@ impl Interceptor for ServerAuthenticationInterceptor {
GrpcAuthentication::None => Ok(request),
GrpcAuthentication::Basic {
username,
password: hashed_password,
} => self.handle_basic_auth(request, username, hashed_password.reveal()),
password: phc_password,
} => self.handle_basic_auth(request, username, phc_password),
}
}
}
Expand Down
31 changes: 0 additions & 31 deletions applications/minotari_console_wallet/src/automation/commands.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use chrono::{DateTime, Utc};
use digest::Digest;
use futures::FutureExt;
use log::*;
use minotari_app_grpc::authentication::salted_password::create_salted_hashed_password;
use minotari_wallet::{
connectivity_service::WalletConnectivityInterface,
output_manager_service::{handle::OutputManagerHandle, UtxoSelectionCriteria},
Expand Down Expand Up @@ -962,36 +961,6 @@ pub async fn command_runner(
eprintln!("RevalidateWalletDb error! {}", e);
}
},
HashGrpcPassword(args) => {
match config
.grpc_authentication
.username_password()
.ok_or_else(|| CommandError::General("GRPC basic auth is not configured".to_string()))
{
Ok((username, password)) => {
match create_salted_hashed_password(password.reveal())
.map_err(|e| CommandError::General(e.to_string()))
{
Ok(hashed_password) => {
if args.short {
println!("{}", *hashed_password);
} else {
println!("Your hashed password is:");
println!("{}", *hashed_password);
println!();
println!(
"Use HTTP basic auth with username '{}' and the hashed password to make GRPC \
requests",
username
);
}
},
Err(e) => eprintln!("HashGrpcPassword error! {}", e),
}
},
Err(e) => eprintln!("HashGrpcPassword error! {}", e),
}
},
RegisterValidatorNode(args) => {
let tx_id = register_validator_node(
args.amount,
Expand Down
7 changes: 0 additions & 7 deletions applications/minotari_console_wallet/src/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ pub enum CliCommands {
FinaliseShaAtomicSwap(FinaliseShaAtomicSwapArgs),
ClaimShaAtomicSwapRefund(ClaimShaAtomicSwapRefundArgs),
RevalidateWalletDb,
HashGrpcPassword(HashPasswordArgs),
RegisterValidatorNode(RegisterValidatorNodeArgs),
}

Expand Down Expand Up @@ -281,12 +280,6 @@ pub struct ClaimShaAtomicSwapRefundArgs {
pub message: String,
}

#[derive(Debug, Args, Clone)]
pub struct HashPasswordArgs {
/// If true, only output the hashed password and the salted password. Otherwise a usage explanation is output.
pub short: bool,
}

#[derive(Debug, Args, Clone)]
pub struct RegisterValidatorNodeArgs {
pub amount: MicroMinotari,
Expand Down
4 changes: 2 additions & 2 deletions applications/minotari_console_wallet/src/wallet_modes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,8 @@ async fn run_grpc(

info!(target: LOG_TARGET, "Starting GRPC on {}", grpc_listener_addr);
let address = multiaddr_to_socketaddr(&grpc_listener_addr).map_err(|e| e.to_string())?;
let auth = ServerAuthenticationInterceptor::new(auth_config);
let auth = ServerAuthenticationInterceptor::new(auth_config)
.ok_or("Unable to prepare server gRPC authentication".to_string())?;
let service = minotari_app_grpc::tari_rpc::wallet_server::WalletServer::with_interceptor(grpc, auth);

Server::builder()
Expand Down Expand Up @@ -481,7 +482,6 @@ mod test {
CliCommands::FinaliseShaAtomicSwap(_) => {},
CliCommands::ClaimShaAtomicSwapRefund(_) => {},
CliCommands::RevalidateWalletDb => {},
CliCommands::HashGrpcPassword(_) => {},
CliCommands::RegisterValidatorNode(_) => {},
}
}
Expand Down
Loading

0 comments on commit 623f127

Please sign in to comment.