From 1d0eab430667d3481a94f7b1ba59fdf6c0832ea4 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 25 Sep 2023 09:29:43 -0500 Subject: [PATCH] Relax format restrictions on authentication tokens --- core/src/task.rs | 236 ++++++++++++++++++++++++++++++++------- tools/src/bin/collect.rs | 6 +- 2 files changed, 199 insertions(+), 43 deletions(-) diff --git a/core/src/task.rs b/core/src/task.rs index 2adaca84f9..ade319af3d 100644 --- a/core/src/task.rs +++ b/core/src/task.rs @@ -1,6 +1,6 @@ use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; use derivative::Derivative; -use http::header::AUTHORIZATION; +use http::{header::AUTHORIZATION, HeaderValue}; use janus_messages::taskprov; use rand::{distributions::Standard, prelude::Distribution}; use ring::constant_time; @@ -622,7 +622,7 @@ pub enum AuthenticationToken { /// The token is not necessarily an OAuth token. /// /// [1]: https://datatracker.ietf.org/doc/html/rfc6750#section-2.1 - Bearer(TokenInner), + Bearer(BearerToken), /// Token presented as the value of the "DAP-Auth-Token" HTTP header. Conforms to /// [draft-dcook-ppm-dap-interop-test-design-03][1], sections [4.3.3][2] and [4.4.2][3], and @@ -632,30 +632,30 @@ pub enum AuthenticationToken { /// [2]: https://datatracker.ietf.org/doc/html/draft-dcook-ppm-dap-interop-test-design-03#section-4.3.3 /// [3]: https://datatracker.ietf.org/doc/html/draft-dcook-ppm-dap-interop-test-design-03#section-4.4.2 /// [4]: https://datatracker.ietf.org/doc/html/draft-ietf-ppm-dap-01#name-https-sender-authentication - DapAuth(TokenInner), + DapAuth(DapAuthToken), } impl AuthenticationToken { /// Attempts to create a new bearer token from the provided bytes. pub fn new_bearer_token_from_bytes>(bytes: T) -> Result { - TokenInner::try_from(bytes.as_ref().to_vec()).map(AuthenticationToken::Bearer) + BearerToken::try_from(bytes.as_ref().to_vec()).map(AuthenticationToken::Bearer) } /// Attempts to create a new bearer token from the provided string pub fn new_bearer_token_from_string>(string: T) -> Result { - TokenInner::try_from_str(string.into()).map(AuthenticationToken::Bearer) + BearerToken::try_from(string.into()).map(AuthenticationToken::Bearer) } /// Attempts to create a new DAP auth token from the provided bytes. pub fn new_dap_auth_token_from_bytes>(bytes: T) -> Result { - TokenInner::try_from(bytes.as_ref().to_vec()).map(AuthenticationToken::DapAuth) + DapAuthToken::try_from(bytes.as_ref().to_vec()).map(AuthenticationToken::DapAuth) } /// Attempts to create a new DAP auth token from the provided string. pub fn new_dap_auth_token_from_string>( string: T, ) -> Result { - TokenInner::try_from_str(string.into()).map(AuthenticationToken::DapAuth) + DapAuthToken::try_from(string.into()).map(AuthenticationToken::DapAuth) } /// Returns an HTTP header and value that should be used to authenticate an HTTP request with @@ -692,71 +692,202 @@ impl Distribution for Standard { } } -/// A token value used to authenticate HTTP requests. +/// A token value used to authenticate HTTP requests. This token is used in the "DAP-Auth-Token" +/// HTTP request header. /// -/// The token is used directly in HTTP request headers without further encoding and so much be a -/// legal HTTP header value. More specifically, the token is restricted to the unpadded, URL-safe -/// Base64 alphabet, as specified in [RFC 4648 section 5][1]. The unpadded, URL-safe Base64 string -/// is the canonical form of the token and is used in configuration files, Janus aggregator API -/// requests and HTTP authentication headers. +/// This token is used directly in HTTP request headers without further encoding and so much be a +/// legal HTTP header value. The literal value is the canonical form of the token and is used +/// directly, without any additional encoding or decoding, in configuration files, Janus aggregator +/// API requests, and HTTP authentication headers. /// /// This opaque type ensures it's impossible to construct an [`AuthenticationToken`] whose contents /// are invalid. -/// -/// [1]: https://datatracker.ietf.org/doc/html/rfc4648#section-5 #[derive(Clone, Derivative, Serialize)] #[derivative(Debug)] #[serde(transparent)] -pub struct TokenInner(#[derivative(Debug = "ignore")] String); +pub struct DapAuthToken(#[derivative(Debug = "ignore")] String); -impl TokenInner { +impl DapAuthToken { /// Returns the token as a string. pub fn as_str(&self) -> &str { &self.0 } - fn try_from_str(value: String) -> Result { - // Verify that the string is legal unpadded, URL-safe Base64 - URL_SAFE_NO_PAD.decode(&value)?; + /// Validate that a DAP-Auth-Token value is a valid HTTP header value. + fn validate(value: &str) -> Result<(), anyhow::Error> { + HeaderValue::try_from(value)?; + Ok(()) + } +} + +impl AsRef for DapAuthToken { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl AsRef<[u8]> for DapAuthToken { + fn as_ref(&self) -> &[u8] { + self.0.as_bytes() + } +} + +impl TryFrom for DapAuthToken { + type Error = anyhow::Error; + + fn try_from(value: String) -> Result { + Self::validate(&value)?; Ok(Self(value)) } +} + +impl TryFrom> for DapAuthToken { + type Error = anyhow::Error; + + fn try_from(value: Vec) -> Result { + Self::try_from(String::from_utf8(value)?) + } +} + +impl<'de> Deserialize<'de> for DapAuthToken { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + String::deserialize(deserializer) + .and_then(|string| Self::try_from(string).map_err(D::Error::custom)) + } +} + +impl PartialEq for DapAuthToken { + fn eq(&self, other: &Self) -> bool { + // We attempt constant-time comparisons of the token data to mitigate timing attacks. Note + // that this function still leaks whether the lengths of the tokens are equal -- this is + // acceptable because we expect the content of the tokens to provide enough randomness that + // needs to be guessed even if the length is known. + constant_time::verify_slices_are_equal(self.0.as_ref(), other.0.as_ref()).is_ok() + } +} + +impl Eq for DapAuthToken {} + +impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> DapAuthToken { + DapAuthToken(URL_SAFE_NO_PAD.encode(rng.gen::<[u8; 16]>())) + } +} + +/// A token value used to authenticate HTTP requests. This token is used in "Authorization: Bearer" +/// HTTP request headers. +/// +/// Token values must follow the syntax in +/// . Its literal value is the canonical +/// form of the token and is used directly, without any additional encoding or decoding, in +/// configuration files, Janus aggregator API requests, and HTTP authentication headers. +/// +/// This opaque type ensures it's impossible to construct an [`AuthenticationToken`] whose contents +/// are invalid. +#[derive(Clone, Derivative, Serialize)] +#[derivative(Debug)] +#[serde(transparent)] +pub struct BearerToken(#[derivative(Debug = "ignore")] String); + +impl BearerToken { + /// Returns the token as a string. + pub fn as_str(&self) -> &str { + &self.0 + } + + /// Validate that a bearer token value matches the format in + /// https://datatracker.ietf.org/doc/html/rfc6750#section-2.1. + fn validate(value: &str) -> Result<(), anyhow::Error> { + let mut iter = value.chars(); + let mut any_non_equals = false; + // First loop: consume "normal" characters, stop when we see an equals sign for padding or + // reach the end of the input. + for c in &mut iter { + match c { + 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '.' | '_' | '~' | '+' | '/' => { + any_non_equals = true; + } + '=' => { + if !any_non_equals { + return Err(anyhow::anyhow!("bearer token may not start with '='")); + } + break; + } + _ => return Err(anyhow::anyhow!("bearer token may not contain '{c}'")), + } + } + // Second loop: consume any further padding characters, if present. + for c in &mut iter { + match c { + '=' => {} + _ => { + return Err(anyhow::anyhow!( + "bearer token may only contain '=' at the end" + )) + } + } + } + Ok(()) + } +} - fn try_from(value: Vec) -> Result { - Self::try_from_str(String::from_utf8(value)?) +impl AsRef for BearerToken { + fn as_ref(&self) -> &str { + &self.0 } } -impl AsRef<[u8]> for TokenInner { +impl AsRef<[u8]> for BearerToken { fn as_ref(&self) -> &[u8] { self.0.as_bytes() } } -impl<'de> Deserialize<'de> for TokenInner { +impl TryFrom for BearerToken { + type Error = anyhow::Error; + + fn try_from(value: String) -> Result { + Self::validate(&value)?; + Ok(Self(value)) + } +} + +impl TryFrom> for BearerToken { + type Error = anyhow::Error; + + fn try_from(value: Vec) -> Result { + Self::try_from(String::from_utf8(value)?) + } +} + +impl<'de> Deserialize<'de> for BearerToken { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, { String::deserialize(deserializer) - .and_then(|string| Self::try_from_str(string).map_err(D::Error::custom)) + .and_then(|string| Self::try_from(string).map_err(D::Error::custom)) } } -impl PartialEq for TokenInner { +impl PartialEq for BearerToken { fn eq(&self, other: &Self) -> bool { // We attempt constant-time comparisons of the token data to mitigate timing attacks. Note - // that this function still eaks whether the lengths of the tokens are equal -- this is - // acceptable because we expec the content of the tokens to provide enough randomness that + // that this function still leaks whether the lengths of the tokens are equal -- this is + // acceptable because we expect the content of the tokens to provide enough randomness that // needs to be guessed even if the length is known. constant_time::verify_slices_are_equal(self.0.as_bytes(), other.0.as_bytes()).is_ok() } } -impl Eq for TokenInner {} +impl Eq for BearerToken {} -impl Distribution for Standard { - fn sample(&self, rng: &mut R) -> TokenInner { - TokenInner(URL_SAFE_NO_PAD.encode(rng.gen::<[u8; 16]>())) +impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> BearerToken { + BearerToken(URL_SAFE_NO_PAD.encode(rng.gen::<[u8; 16]>())) } } @@ -863,14 +994,39 @@ mod tests { ); } - #[rstest::rstest] - #[case::dap_auth("DapAuth")] - #[case::bearer("Bearer")] #[test] - fn reject_invalid_auth_token(#[case] token_type: &str) { - serde_yaml::from_str::(&format!( - "{{type: \"{token_type}\", token: \"é\"}}" - )) - .unwrap_err(); + fn valid_dap_auth_token() { + serde_yaml::from_str::( + "{type: \"DapAuth\", token: \"correct-horse-battery-staple-!@#$\"}", + ) + .unwrap(); + } + + #[test] + fn valid_bearer_token() { + serde_yaml::from_str::( + "{type: \"Bearer\", token: \"AAAAAAA~-_/A===\"}", + ) + .unwrap(); + } + + #[test] + fn reject_invalid_auth_token_dap_auth() { + serde_yaml::from_str::("{type: \"DapAuth\", token: \"\\x0b\"}") + .unwrap_err(); + serde_yaml::from_str::("{type: \"DapAuth\", token: \"\\x00\"}") + .unwrap_err(); + } + + #[test] + fn reject_invalid_auth_token_bearer() { + serde_yaml::from_str::("{type: \"Bearer\", token: \"é\"}") + .unwrap_err(); + serde_yaml::from_str::("{type: \"Bearer\", token: \"^\"}") + .unwrap_err(); + serde_yaml::from_str::("{type: \"Bearer\", token: \"=\"}") + .unwrap_err(); + serde_yaml::from_str::("{type: \"Bearer\", token: \"AAAA==AAA\"}") + .unwrap_err(); } } diff --git a/tools/src/bin/collect.rs b/tools/src/bin/collect.rs index 739738d45f..3d6b6a5741 100644 --- a/tools/src/bin/collect.rs +++ b/tools/src/bin/collect.rs @@ -603,7 +603,7 @@ mod tests { test_util::{generate_test_hpke_config_and_private_key, SAMPLE_DIVVIUP_HPKE_CONFIG}, DivviUpHpkeConfig, HpkeKeypair, }, - task::TokenInner, + task::{BearerToken, DapAuthToken}, }; use janus_messages::{BatchId, TaskId}; use prio::codec::Encode; @@ -1051,8 +1051,8 @@ mod tests { "--vdaf=count".to_string(), ]); - let dap_auth_token: TokenInner = random(); - let bearer_token: TokenInner = random(); + let dap_auth_token: DapAuthToken = random(); + let bearer_token: BearerToken = random(); let dap_auth_token_argument = format!("--dap-auth-token={}", dap_auth_token.as_str()); let authorization_bearer_token_argument =