From 748591691bf787bee8aafd380ac3d5c5872d82e5 Mon Sep 17 00:00:00 2001 From: Georgios Konstantopoulos Date: Fri, 20 Dec 2019 11:40:38 +0200 Subject: [PATCH] fix(api/http): Use SecretStrings for HTTP authorization secrets Fixes https://github.com/interledger-rs/interledger-rs/issues/556 --- Cargo.lock | 16 ++-- crates/interledger-api/Cargo.toml | 2 +- crates/interledger-api/src/routes/accounts.rs | 79 +++++++++++-------- .../src/routes/node_settings.rs | 19 +++-- crates/interledger-http/Cargo.toml | 2 +- crates/interledger-http/src/server.rs | 12 ++- 6 files changed, 74 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ee0dad52f..d3a31050b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -751,7 +751,7 @@ dependencies = [ "redis 0.13.0 (registry+https://github.com/rust-lang/crates.io-index)", "reqwest 0.9.22 (registry+https://github.com/rust-lang/crates.io-index)", "ring 0.16.9 (registry+https://github.com/rust-lang/crates.io-index)", - "secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.1.22 (registry+https://github.com/rust-lang/crates.io-index)", @@ -832,7 +832,7 @@ dependencies = [ "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "reqwest 0.9.22 (registry+https://github.com/rust-lang/crates.io-index)", - "secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", "serde_path_to_error 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", @@ -859,7 +859,7 @@ dependencies = [ "parking_lot 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "quick-error 1.2.2 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", - "secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "stream-cancel 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.1.22 (registry+https://github.com/rust-lang/crates.io-index)", "tokio-executor 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", @@ -906,7 +906,7 @@ dependencies = [ "mime 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)", "regex 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "reqwest 0.9.22 (registry+https://github.com/rust-lang/crates.io-index)", - "secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", "serde_path_to_error 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", @@ -990,7 +990,7 @@ dependencies = [ "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "reqwest 0.9.22 (registry+https://github.com/rust-lang/crates.io-index)", "ring 0.16.9 (registry+https://github.com/rust-lang/crates.io-index)", - "secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.1.22 (registry+https://github.com/rust-lang/crates.io-index)", "tokio-executor 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1074,7 +1074,7 @@ dependencies = [ "rand 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", "redis 0.13.0 (registry+https://github.com/rust-lang/crates.io-index)", "ring 0.16.9 (registry+https://github.com/rust-lang/crates.io-index)", - "secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.1.22 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1924,7 +1924,7 @@ dependencies = [ [[package]] name = "secrecy" -version = "0.5.1" +version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bytes 0.4.12 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3067,7 +3067,7 @@ dependencies = [ "checksum scoped-tls 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ea6a9290e3c9cf0f18145ef7ffa62d68ee0bf5fcd651017e586dc7fd5da448c2" "checksum scopeguard 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b42e15e59b18a828bbf5c58ea01debb36b9b096346de35d941dcb89009f24a0d" "checksum sct 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)" = "e3042af939fca8c3453b7af0f1c66e533a15a86169e39de2657310ade8f98d3c" -"checksum secrecy 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "30e17355cc3cbc217e0bad1a71b38c60ed9ee34bbc3ede7ffb1c4af20f0a14c9" +"checksum secrecy 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)" = "f2309a011083016deb67a984e8edb0845e42b4c6aaadae7658ebdcb47b91fdbc" "checksum security-framework 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "eee63d0f4a9ec776eeb30e220f0bc1e092c3ad744b2a379e3993070364d3adc2" "checksum security-framework-sys 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "9636f8989cbf61385ae4824b98c1aaa54c994d7d8b41f11c601ed799f0549a56" "checksum semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "1d7eb9ef2c18661902cc47e535f9bc51b78acd254da71d375c2f6720d9a40403" diff --git a/crates/interledger-api/Cargo.toml b/crates/interledger-api/Cargo.toml index 990c895af..6d32c8850 100644 --- a/crates/interledger-api/Cargo.toml +++ b/crates/interledger-api/Cargo.toml @@ -31,7 +31,7 @@ reqwest = { version = "0.9.22", default-features = false, features = ["default-t url = { version = "2.1.0", default-features = false, features = ["serde"] } uuid = { version = "0.8.1", default-features = false} warp = { version = "0.1.20", default-features = false } -secrecy = { version = "0.5.1", default-features = false, features = ["serde"] } +secrecy = { version = "0.5.2", default-features = false, features = ["serde"] } lazy_static = "1.4.0" [badges] diff --git a/crates/interledger-api/src/routes/accounts.rs b/crates/interledger-api/src/routes/accounts.rs index e10fec7a9..bca63e768 100644 --- a/crates/interledger-api/src/routes/accounts.rs +++ b/crates/interledger-api/src/routes/accounts.rs @@ -18,6 +18,7 @@ use interledger_settlement::core::types::SettlementAccount; use interledger_spsp::{pay, SpspResponder}; use interledger_stream::{PaymentNotification, StreamNotificationsStore}; use log::{debug, error, trace}; +use secrecy::{ExposeSecret, SecretString}; use serde::{Deserialize, Serialize}; use serde_json::json; use std::convert::TryFrom; @@ -66,14 +67,16 @@ where // Helper filters let admin_auth_header = format!("Bearer {}", admin_api_token); - let admin_only = warp::header::("authorization") - .and_then(move |authorization| -> Result<(), Rejection> { - if authorization == admin_auth_header { - Ok(()) - } else { - Err(ApiError::unauthorized().into()) - } - }) + let admin_only = warp::header::("authorization") + .and_then( + move |authorization: SecretString| -> Result<(), Rejection> { + if authorization.expose_secret() == &admin_auth_header { + Ok(()) + } else { + Err(ApiError::unauthorized().into()) + } + }, + ) // This call makes it so we do not pass on a () value on // success to the next filter, it just gets rid of it .untuple_one() @@ -121,12 +124,15 @@ where // unauthorized that the the request actually should not cause. // This function needs parameters which can be prepared by `account_username`. let admin_or_authorized_user_only = warp::filters::ext::get::() - .and(warp::header::("authorization")) + .and(warp::header::("authorization")) .and(with_store.clone()) .and(with_admin_auth_header.clone()) .and_then( - |path_username: Username, auth_string: String, store: S, admin_auth_header: String| { - if auth_string.len() < BEARER_TOKEN_START { + |path_username: Username, + auth_string: SecretString, + store: S, + admin_auth_header: String| { + if auth_string.expose_secret().len() < BEARER_TOKEN_START { return Either::A(err(ApiError::bad_request().into())); } Either::B(store.get_account_id_from_username(&path_username).then( @@ -137,14 +143,14 @@ where )); } let account_id = account_id.unwrap(); - if auth_string == admin_auth_header { + if auth_string.expose_secret() == &admin_auth_header { return Either::A(ok(account_id)); } Either::B( store .get_account_from_http_auth( &path_username, - &auth_string[BEARER_TOKEN_START..], + &auth_string.expose_secret()[BEARER_TOKEN_START..], ) .then(move |authorized_account: Result| { if authorized_account.is_err() { @@ -167,28 +173,33 @@ where // The same structure as `admin_or_authorized_user_only`. // This function needs parameters which can be prepared by `account_username`. let authorized_user_only = warp::filters::ext::get::() - .and(warp::header::("authorization")) + .and(warp::header::("authorization")) .and(with_store.clone()) - .and_then(|path_username: Username, auth_string: String, store: S| { - if auth_string.len() < BEARER_TOKEN_START { - return Either::A(err(ApiError::bad_request().into())); - } - Either::B( - store - .get_account_from_http_auth(&path_username, &auth_string[BEARER_TOKEN_START..]) - .then(move |authorized_account: Result| { - if authorized_account.is_err() { - return err::(ApiError::unauthorized().into()); - } - let authorized_account = authorized_account.unwrap(); - if &path_username == authorized_account.username() { - ok(authorized_account) - } else { - err(ApiError::unauthorized().into()) - } - }), - ) - }) + .and_then( + |path_username: Username, auth_string: SecretString, store: S| { + if auth_string.expose_secret().len() < BEARER_TOKEN_START { + return Either::A(err(ApiError::bad_request().into())); + } + Either::B( + store + .get_account_from_http_auth( + &path_username, + &auth_string.expose_secret()[BEARER_TOKEN_START..], + ) + .then(move |authorized_account: Result| { + if authorized_account.is_err() { + return err::(ApiError::unauthorized().into()); + } + let authorized_account = authorized_account.unwrap(); + if &path_username == authorized_account.username() { + ok(authorized_account) + } else { + err(ApiError::unauthorized().into()) + } + }), + ) + }, + ) .boxed(); // POST /accounts diff --git a/crates/interledger-api/src/routes/node_settings.rs b/crates/interledger-api/src/routes/node_settings.rs index 4a269761e..40f12819b 100644 --- a/crates/interledger-api/src/routes/node_settings.rs +++ b/crates/interledger-api/src/routes/node_settings.rs @@ -11,6 +11,7 @@ use interledger_service::{Account, Username}; use interledger_service_util::{BalanceStore, ExchangeRateStore}; use interledger_settlement::core::types::SettlementAccount; use log::{error, trace}; +use secrecy::{ExposeSecret, SecretString}; use serde::Serialize; use std::{ collections::HashMap, @@ -44,14 +45,16 @@ where { // Helper filters let admin_auth_header = format!("Bearer {}", admin_api_token); - let admin_only = warp::header::("authorization") - .and_then(move |authorization| -> Result<(), Rejection> { - if authorization == admin_auth_header { - Ok(()) - } else { - Err(ApiError::unauthorized().into()) - } - }) + let admin_only = warp::header::("authorization") + .and_then( + move |authorization: SecretString| -> Result<(), Rejection> { + if authorization.expose_secret() == &admin_auth_header { + Ok(()) + } else { + Err(ApiError::unauthorized().into()) + } + }, + ) // This call makes it so we do not pass on a () value on // success to the next filter, it just gets rid of it .untuple_one() diff --git a/crates/interledger-http/Cargo.toml b/crates/interledger-http/Cargo.toml index ac0f6d96c..80e660536 100644 --- a/crates/interledger-http/Cargo.toml +++ b/crates/interledger-http/Cargo.toml @@ -24,7 +24,7 @@ chrono = { version = "0.4.9", features = ["clock"], default-features = false } regex = { version ="1.3.1", default-features = false, features = ["std"] } lazy_static = { version ="1.4.0", default-features = false } mime = { version ="0.3.14", default-features = false } -secrecy = "0.5.1" +secrecy = "0.5.2" [dev-dependencies] uuid = { version = "0.8.1", features=["v4"]} diff --git a/crates/interledger-http/src/server.rs b/crates/interledger-http/src/server.rs index 65d843abc..1700a1f3e 100644 --- a/crates/interledger-http/src/server.rs +++ b/crates/interledger-http/src/server.rs @@ -8,6 +8,7 @@ use interledger_packet::Prepare; use interledger_service::Username; use interledger_service::{IncomingRequest, IncomingService}; use log::error; +use secrecy::{ExposeSecret, SecretString}; use std::{convert::TryFrom, net::SocketAddr}; use warp::{self, Filter, Rejection}; @@ -44,14 +45,17 @@ where .and(warp::path::param2::()) .and(warp::path("ilp")) .and(warp::path::end()) - .and(warp::header::("authorization")) - .and_then(move |path_username: Username, password: String| { - if password.len() < BEARER_TOKEN_START { + .and(warp::header::("authorization")) + .and_then(move |path_username: Username, password: SecretString| { + if password.expose_secret().len() < BEARER_TOKEN_START { return Either::A(err(ApiError::bad_request().into())); } Either::B( store - .get_account_from_http_auth(&path_username, &password[BEARER_TOKEN_START..]) + .get_account_from_http_auth( + &path_username, + &password.expose_secret()[BEARER_TOKEN_START..], + ) .map_err(move |_| -> Rejection { error!("Invalid authorization provided for user: {}", path_username); ApiError::unauthorized().into()