Merge pull request #11350 from Turbo87/beware-of-the-bears

Allow empty and `Bearer` auth schemes in `Authorization` headers

Author: Turbo87

crates/crates_io_trustpub/src/access_token.rs

@@ -2,6 +2,7 @@ use rand::distr::{Alphanumeric, SampleString};
 use secrecy::{ExposeSecret, SecretString};
 use sha2::digest::Output;
 use sha2::{Digest, Sha256};
+use std::str::FromStr;
 
 /// A temporary access token used to publish crates to crates.io using
 /// the "Trusted Publishing" feature.
@@ -29,19 +30,37 @@ impl AccessToken {
         Self(raw.into())
     }
 
-    /// Parse a byte string into an access token.
+    /// Wrap the raw access token with the token prefix and a checksum.
     ///
-    /// This can be used to convert an HTTP header value into an access token.
-    pub fn from_byte_str(byte_str: &[u8]) -> Result<Self, AccessTokenError> {
-        let suffix = byte_str
-            .strip_prefix(Self::PREFIX.as_bytes())
+    /// This turns e.g. `ABC` into `cio_tp_ABC{checksum}`.
+    pub fn finalize(&self) -> SecretString {
+        let raw = self.0.expose_secret();
+        let checksum = checksum(raw.as_bytes());
+        format!("{}{raw}{checksum}", Self::PREFIX).into()
+    }
+
+    /// Generate a SHA256 hash of the access token.
+    ///
+    /// This is used to create a hashed version of the token for storage in
+    /// the database to avoid storing the plaintext token.
+    pub fn sha256(&self) -> Output<Sha256> {
+        Sha256::digest(self.0.expose_secret())
+    }
+}
+
+impl FromStr for AccessToken {
+    type Err = AccessTokenError;
+
+    /// Parse a string into an access token.
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let suffix = s
+            .strip_prefix(Self::PREFIX)
             .ok_or(AccessTokenError::MissingPrefix)?;
 
         if suffix.len() != Self::RAW_LENGTH + 1 {
             return Err(AccessTokenError::InvalidLength);
         }
 
-        let suffix = std::str::from_utf8(suffix).map_err(|_| AccessTokenError::InvalidCharacter)?;
         if !suffix.chars().all(|c| char::is_ascii_alphanumeric(&c)) {
             return Err(AccessTokenError::InvalidCharacter);
         }
@@ -58,23 +77,6 @@ impl AccessToken {
 
         Ok(Self(raw.into()))
     }
-
-    /// Wrap the raw access token with the token prefix and a checksum.
-    ///
-    /// This turns e.g. `ABC` into `cio_tp_ABC{checksum}`.
-    pub fn finalize(&self) -> SecretString {
-        let raw = self.0.expose_secret();
-        let checksum = checksum(raw.as_bytes());
-        format!("{}{raw}{checksum}", Self::PREFIX).into()
-    }
-
-    /// Generate a SHA256 hash of the access token.
-    ///
-    /// This is used to create a hashed version of the token for storage in
-    /// the database to avoid storing the plaintext token.
-    pub fn sha256(&self) -> Output<Sha256> {
-        Sha256::digest(self.0.expose_secret())
-    }
 }
 
 /// The error type for parsing access tokens.
@@ -129,42 +131,33 @@ mod tests {
     }
 
     #[test]
-    fn test_from_byte_str() {
+    fn test_from_str() {
         let token = AccessToken::generate().finalize();
         let token = token.expose_secret();
-        let token2 = assert_ok!(AccessToken::from_byte_str(token.as_bytes()));
+        let token2 = assert_ok!(token.parse::<AccessToken>());
         assert_eq!(token2.finalize().expose_secret(), token);
 
-        let bytes = b"cio_tp_0000000000000000000000000000000w";
-        assert_ok!(AccessToken::from_byte_str(bytes));
+        let str = "cio_tp_0000000000000000000000000000000w";
+        assert_ok!(str.parse::<AccessToken>());
 
-        let bytes = b"invalid_token";
-        assert_err_eq!(
-            AccessToken::from_byte_str(bytes),
-            AccessTokenError::MissingPrefix
-        );
+        let str = "invalid_token";
+        assert_err_eq!(str.parse::<AccessToken>(), AccessTokenError::MissingPrefix);
 
-        let bytes = b"cio_tp_invalid_token";
-        assert_err_eq!(
-            AccessToken::from_byte_str(bytes),
-            AccessTokenError::InvalidLength
-        );
+        let str = "cio_tp_invalid_token";
+        assert_err_eq!(str.parse::<AccessToken>(), AccessTokenError::InvalidLength);
 
-        let bytes = b"cio_tp_00000000000000000000000000";
-        assert_err_eq!(
-            AccessToken::from_byte_str(bytes),
-            AccessTokenError::InvalidLength
-        );
+        let str = "cio_tp_00000000000000000000000000";
+        assert_err_eq!(str.parse::<AccessToken>(), AccessTokenError::InvalidLength);
 
-        let bytes = b"cio_tp_000000@0000000000000000000000000";
+        let str = "cio_tp_000000@0000000000000000000000000";
         assert_err_eq!(
-            AccessToken::from_byte_str(bytes),
+            str.parse::<AccessToken>(),
             AccessTokenError::InvalidCharacter
         );
 
-        let bytes = b"cio_tp_00000000000000000000000000000000";
+        let str = "cio_tp_00000000000000000000000000000000";
         assert_err_eq!(
-            AccessToken::from_byte_str(bytes),
+            str.parse::<AccessToken>(),
             AccessTokenError::InvalidChecksum {
                 claimed: '0',
                 actual: 'w',

src/auth.rs

@@ -4,14 +4,63 @@ use crate::middleware::log_request::RequestLogExt;
 use crate::models::token::{CrateScope, EndpointScope};
 use crate::models::{ApiToken, User};
 use crate::util::errors::{
-    AppResult, InsecurelyGeneratedTokenRevoked, account_locked, forbidden, internal,
+    AppResult, BoxedAppError, InsecurelyGeneratedTokenRevoked, account_locked, custom, forbidden,
+    internal,
 };
 use crate::util::token::HashedToken;
+use axum::extract::FromRequestParts;
 use chrono::Utc;
 use crates_io_session::SessionExtension;
 use diesel_async::AsyncPgConnection;
-use http::header;
 use http::request::Parts;
+use http::{StatusCode, header};
+use secrecy::{ExposeSecret, SecretString};
+
+pub struct AuthHeader(SecretString);
+
+impl AuthHeader {
+    pub async fn optional_from_request_parts(parts: &Parts) -> Result<Option<Self>, BoxedAppError> {
+        let Some(auth_header) = parts.headers.get(header::AUTHORIZATION) else {
+            return Ok(None);
+        };
+
+        let auth_header = auth_header.to_str().map_err(|_| {
+            let message = "Invalid `Authorization` header: Found unexpected non-ASCII characters";
+            custom(StatusCode::UNAUTHORIZED, message)
+        })?;
+
+        let (scheme, token) = auth_header.split_once(' ').unwrap_or(("", auth_header));
+        if !(scheme.eq_ignore_ascii_case("Bearer") || scheme.is_empty()) {
+            let message = format!(
+                "Invalid `Authorization` header: Found unexpected authentication scheme `{scheme}`"
+            );
+            return Err(custom(StatusCode::UNAUTHORIZED, message));
+        }
+
+        let token = SecretString::from(token.trim_ascii());
+        Ok(Some(AuthHeader(token)))
+    }
+
+    pub async fn from_request_parts(parts: &Parts) -> Result<Self, BoxedAppError> {
+        let auth = Self::optional_from_request_parts(parts).await?;
+        auth.ok_or_else(|| {
+            let message = "Missing `Authorization` header";
+            custom(StatusCode::UNAUTHORIZED, message)
+        })
+    }
+
+    pub fn token(&self) -> &SecretString {
+        &self.0
+    }
+}
+
+impl<S: Send + Sync> FromRequestParts<S> for AuthHeader {
+    type Rejection = BoxedAppError;
+
+    async fn from_request_parts(parts: &mut Parts, _: &S) -> Result<Self, Self::Rejection> {
+        Self::from_request_parts(parts).await
+    }
+}
 
 #[derive(Debug, Clone)]
 pub struct AuthCheck {
@@ -203,17 +252,12 @@ async fn authenticate_via_token(
     parts: &Parts,
     conn: &mut AsyncPgConnection,
 ) -> AppResult<Option<TokenAuthentication>> {
-    let maybe_authorization = parts
-        .headers()
-        .get(header::AUTHORIZATION)
-        .and_then(|h| h.to_str().ok());
-
-    let Some(header_value) = maybe_authorization else {
+    let Some(auth_header) = AuthHeader::optional_from_request_parts(parts).await? else {
         return Ok(None);
     };
 
-    let token =
-        HashedToken::parse(header_value).map_err(|_| InsecurelyGeneratedTokenRevoked::boxed())?;
+    let token = auth_header.token().expose_secret();
+    let token = HashedToken::parse(token).map_err(|_| InsecurelyGeneratedTokenRevoked::boxed())?;
 
     let token = ApiToken::find_by_api_token(conn, &token)
         .await

src/controllers/krate/publish.rs

@@ -1,7 +1,7 @@
 //! Functionality related to publishing a new crate or version of a crate.
 
 use crate::app::AppState;
-use crate::auth::{AuthCheck, Authentication};
+use crate::auth::{AuthCheck, AuthHeader, Authentication};
 use crate::worker::jobs::{
     self, CheckTyposquat, SendPublishNotificationsJob, UpdateDefaultVersion,
 };
@@ -19,8 +19,9 @@ use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
 use futures_util::TryFutureExt;
 use futures_util::TryStreamExt;
 use hex::ToHex;
+use http::StatusCode;
 use http::request::Parts;
-use http::{StatusCode, header};
+use secrecy::ExposeSecret;
 use sha2::{Digest, Sha256};
 use std::collections::HashMap;
 use tokio::io::{AsyncRead, AsyncReadExt};
@@ -146,21 +147,20 @@ pub async fn publish(app: AppState, req: Parts, body: Body) -> AppResult<Json<Go
         .await
         .optional()?;
 
-    // Trusted publishing tokens are distinguished from regular crates.io API
-    // tokens because they use the `Bearer` auth scheme, so we look for that
-    // specific prefix.
-    let trustpub_token = req
-        .headers
-        .get(header::AUTHORIZATION)
-        .and_then(|h| {
-            let mut split = h.as_bytes().splitn(2, |b| *b == b' ');
-            Some((split.next()?, split.next()?))
+    let auth_header = AuthHeader::optional_from_request_parts(&req).await?;
+    let trustpub_token = auth_header
+        .and_then(|auth| {
+            let token = auth.token().expose_secret();
+            if !token.starts_with(AccessToken::PREFIX) {
+                return None;
+            }
+
+            Some(token.parse::<AccessToken>().map_err(|_| {
+                let message = "Invalid `Authorization` header: Failed to parse token";
+                custom(StatusCode::UNAUTHORIZED, message)
+            }))
         })
-        .filter(|(scheme, _token)| scheme.eq_ignore_ascii_case(b"Bearer"))
-        .map(|(_scheme, token)| token.trim_ascii())
-        .map(AccessToken::from_byte_str)
-        .transpose()
-        .map_err(|_| forbidden("Invalid authentication token"))?;
+        .transpose()?;
 
     let auth = if let Some(trustpub_token) = trustpub_token {
         let Some(existing_crate) = &existing_crate else {

src/controllers/trustpub/tokens/exchange/tests.rs

@@ -83,7 +83,7 @@ async fn test_happy_path() -> anyhow::Result<()> {
     "#);
 
     let token = json["token"].as_str().unwrap();
-    let token = assert_ok!(AccessToken::from_byte_str(token.as_bytes()));
+    let token = assert_ok!(token.parse::<AccessToken>());
     let hashed_token = token.sha256();
 
     let mut conn = client.app().db_conn().await;

src/controllers/trustpub/tokens/revoke/mod.rs

@@ -1,10 +1,12 @@
 use crate::app::AppState;
+use crate::auth::AuthHeader;
 use crate::util::errors::{AppResult, custom};
 use crates_io_database::schema::trustpub_tokens;
 use crates_io_trustpub::access_token::AccessToken;
 use diesel::prelude::*;
 use diesel_async::RunQueryDsl;
-use http::{HeaderMap, StatusCode, header};
+use http::StatusCode;
+use secrecy::ExposeSecret;
 
 #[cfg(test)]
 mod tests;
@@ -20,19 +22,10 @@ mod tests;
     tag = "trusted_publishing",
     responses((status = 204, description = "Successful Response")),
 )]
-pub async fn revoke_trustpub_token(app: AppState, headers: HeaderMap) -> AppResult<StatusCode> {
-    let Some(auth_header) = headers.get(header::AUTHORIZATION) else {
-        let message = "Missing authorization header";
-        return Err(custom(StatusCode::UNAUTHORIZED, message));
-    };
-
-    let Some(bearer) = auth_header.as_bytes().strip_prefix(b"Bearer ") else {
-        let message = "Invalid authorization header";
-        return Err(custom(StatusCode::UNAUTHORIZED, message));
-    };
-
-    let Ok(token) = AccessToken::from_byte_str(bearer) else {
-        let message = "Invalid authorization header";
+pub async fn revoke_trustpub_token(app: AppState, auth: AuthHeader) -> AppResult<StatusCode> {
+    let token = auth.token().expose_secret();
+    let Ok(token) = token.parse::<AccessToken>() else {
+        let message = "Invalid `Authorization` header: Failed to parse token";
         return Err(custom(StatusCode::UNAUTHORIZED, message));
     };
 

src/controllers/trustpub/tokens/revoke/tests.rs

@@ -61,13 +61,34 @@ async fn test_happy_path() -> anyhow::Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread")]
+async fn test_happy_path_without_auth_scheme() -> anyhow::Result<()> {
+    let (app, _client) = TestApp::full().empty().await;
+    let mut conn = app.db_conn().await;
+
+    let token1 = new_token(&mut conn, 1).await?;
+    let _token2 = new_token(&mut conn, 2).await?;
+    assert_compact_debug_snapshot!(all_crate_ids(&mut conn).await?, @"[[Some(1)], [Some(2)]]");
+
+    let token_client = MockTokenUser::with_auth_header(token1, app.clone());
+
+    let response = token_client.delete::<()>(URL).await;
+    assert_snapshot!(response.status(), @"204 No Content");
+    assert_eq!(response.text(), "");
+
+    // Check that the token is deleted
+    assert_compact_debug_snapshot!(all_crate_ids(&mut conn).await?, @"[[Some(2)]]");
+
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread")]
 async fn test_missing_authorization_header() -> anyhow::Result<()> {
     let (_app, client) = TestApp::full().empty().await;
 
     let response = client.delete::<()>(URL).await;
     assert_snapshot!(response.status(), @"401 Unauthorized");
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Missing authorization header"}]}"#);
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Missing `Authorization` header"}]}"#);
 
     Ok(())
 }
@@ -82,7 +103,7 @@ async fn test_invalid_authorization_header_format() -> anyhow::Result<()> {
 
     let response = token_client.delete::<()>(URL).await;
     assert_snapshot!(response.status(), @"401 Unauthorized");
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Invalid authorization header"}]}"#);
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Invalid `Authorization` header: Failed to parse token"}]}"#);
 
     Ok(())
 }
@@ -97,7 +118,7 @@ async fn test_invalid_token_format() -> anyhow::Result<()> {
 
     let response = token_client.delete::<()>(URL).await;
     assert_snapshot!(response.status(), @"401 Unauthorized");
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Invalid authorization header"}]}"#);
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Invalid `Authorization` header: Failed to parse token"}]}"#);
 
     Ok(())
 }