diff --git a/crates/crates_io_trustpub/src/access_token.rs b/crates/crates_io_trustpub/src/access_token.rs index bdb9c2ed9fc..46b2877e348 100644 --- a/crates/crates_io_trustpub/src/access_token.rs +++ b/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 { - 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::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 { + 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::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::()); 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::()); - let bytes = b"invalid_token"; - assert_err_eq!( - AccessToken::from_byte_str(bytes), - AccessTokenError::MissingPrefix - ); + let str = "invalid_token"; + assert_err_eq!(str.parse::(), 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::(), 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::(), 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::(), AccessTokenError::InvalidCharacter ); - let bytes = b"cio_tp_00000000000000000000000000000000"; + let str = "cio_tp_00000000000000000000000000000000"; assert_err_eq!( - AccessToken::from_byte_str(bytes), + str.parse::(), AccessTokenError::InvalidChecksum { claimed: '0', actual: 'w', diff --git a/src/auth.rs b/src/auth.rs index a3eb7d999eb..53c60d5c191 100644 --- a/src/auth.rs +++ b/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, 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 { + 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 FromRequestParts for AuthHeader { + type Rejection = BoxedAppError; + + async fn from_request_parts(parts: &mut Parts, _: &S) -> Result { + 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> { - 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 diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index c635b6be596..1aa770be521 100644 --- a/src/controllers/krate/publish.rs +++ b/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().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 { diff --git a/src/controllers/trustpub/tokens/exchange/tests.rs b/src/controllers/trustpub/tokens/exchange/tests.rs index 8f5d8ac915d..61c5bc08c3b 100644 --- a/src/controllers/trustpub/tokens/exchange/tests.rs +++ b/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::()); let hashed_token = token.sha256(); let mut conn = client.app().db_conn().await; diff --git a/src/controllers/trustpub/tokens/revoke/mod.rs b/src/controllers/trustpub/tokens/revoke/mod.rs index 54693af4b9c..b0d79b2eb45 100644 --- a/src/controllers/trustpub/tokens/revoke/mod.rs +++ b/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 { - 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 { + let token = auth.token().expose_secret(); + let Ok(token) = token.parse::() else { + let message = "Invalid `Authorization` header: Failed to parse token"; return Err(custom(StatusCode::UNAUTHORIZED, message)); }; diff --git a/src/controllers/trustpub/tokens/revoke/tests.rs b/src/controllers/trustpub/tokens/revoke/tests.rs index bac0ef954e0..4b9fd427a41 100644 --- a/src/controllers/trustpub/tokens/revoke/tests.rs +++ b/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(()) } diff --git a/src/tests/krate/publish/auth.rs b/src/tests/krate/publish/auth.rs index 14815909ae6..ebb6f42ccc4 100644 --- a/src/tests/krate/publish/auth.rs +++ b/src/tests/krate/publish/auth.rs @@ -1,10 +1,10 @@ use crate::schema::api_tokens; use crate::tests::builders::{CrateBuilder, PublishBuilder}; -use crate::tests::util::{RequestHelper, TestApp}; +use crate::tests::util::{MockTokenUser, RequestHelper, TestApp}; use diesel::ExpressionMethods; use diesel_async::RunQueryDsl; use googletest::prelude::*; -use insta::assert_snapshot; +use insta::{assert_json_snapshot, assert_snapshot}; #[tokio::test(flavor = "multi_thread")] async fn new_wrong_token() { @@ -54,3 +54,27 @@ async fn new_krate_wrong_user() { assert_that!(app.stored_files().await, empty()); assert_that!(app.emails().await, empty()); } + +#[tokio::test(flavor = "multi_thread")] +async fn new_krate_with_bearer_token() { + let (app, _, _, token) = TestApp::full().with_token().await; + + let token = format!("Bearer {}", token.plaintext()); + let token = MockTokenUser::with_auth_header(token, app.clone()); + + let crate_to_publish = PublishBuilder::new("foo_new", "1.0.0"); + let response = token.publish_crate(crate_to_publish).await; + assert_snapshot!(response.status(), @"200 OK"); + assert_json_snapshot!(response.json(), { + ".crate.created_at" => "[datetime]", + ".crate.updated_at" => "[datetime]", + }); + + assert_snapshot!(app.stored_files().await.join("\n"), @r" + crates/foo_new/foo_new-1.0.0.crate + index/fo/o_/foo_new + rss/crates.xml + rss/crates/foo_new.xml + rss/updates.xml + "); +} diff --git a/src/tests/krate/publish/snapshots/crates_io__tests__krate__publish__auth__new_krate_with_bearer_token-2.snap b/src/tests/krate/publish/snapshots/crates_io__tests__krate__publish__auth__new_krate_with_bearer_token-2.snap new file mode 100644 index 00000000000..88cc12fe00c --- /dev/null +++ b/src/tests/krate/publish/snapshots/crates_io__tests__krate__publish__auth__new_krate_with_bearer_token-2.snap @@ -0,0 +1,42 @@ +--- +source: src/tests/krate/publish/auth.rs +expression: response.json() +--- +{ + "crate": { + "badges": [], + "categories": null, + "created_at": "[datetime]", + "default_version": "1.0.0", + "description": "description", + "documentation": null, + "downloads": 0, + "exact_match": false, + "homepage": null, + "id": "foo_new", + "keywords": null, + "links": { + "owner_team": "/api/v1/crates/foo_new/owner_team", + "owner_user": "/api/v1/crates/foo_new/owner_user", + "owners": "/api/v1/crates/foo_new/owners", + "reverse_dependencies": "/api/v1/crates/foo_new/reverse_dependencies", + "version_downloads": "/api/v1/crates/foo_new/downloads", + "versions": "/api/v1/crates/foo_new/versions" + }, + "max_stable_version": "1.0.0", + "max_version": "1.0.0", + "name": "foo_new", + "newest_version": "1.0.0", + "num_versions": 1, + "recent_downloads": null, + "repository": null, + "updated_at": "[datetime]", + "versions": null, + "yanked": false + }, + "warnings": { + "invalid_badges": [], + "invalid_categories": [], + "other": [] + } +} diff --git a/src/tests/krate/publish/trustpub.rs b/src/tests/krate/publish/trustpub.rs index 8a39abc49f3..1f180f69dcc 100644 --- a/src/tests/krate/publish/trustpub.rs +++ b/src/tests/krate/publish/trustpub.rs @@ -122,8 +122,7 @@ async fn test_full_flow() -> anyhow::Result<()> { // Step 4: Publish a new version of the crate using the temporary access token - let header = format!("Bearer {}", token); - let oidc_token_client = MockTokenUser::with_auth_header(header, app.clone()); + let oidc_token_client = MockTokenUser::with_auth_header(token.to_string(), app.clone()); let pb = PublishBuilder::new(CRATE_NAME, "1.1.0"); let response = oidc_token_client.publish_crate(pb).await; @@ -186,8 +185,7 @@ async fn test_happy_path() -> anyhow::Result<()> { let token = new_token(&mut conn, krate.id).await?; - let header = format!("Bearer {}", token); - let oidc_token_client = MockTokenUser::with_auth_header(header, app); + let oidc_token_client = MockTokenUser::with_auth_header(token, app); let pb = PublishBuilder::new(&krate.name, "1.1.0"); let response = oidc_token_client.publish_crate(pb).await; @@ -226,7 +224,7 @@ async fn test_happy_path_with_fancy_auth_header() -> anyhow::Result<()> { } #[tokio::test(flavor = "multi_thread")] -async fn test_invalid_authorization_header_format() -> anyhow::Result<()> { +async fn test_invalid_token_format() -> anyhow::Result<()> { let (app, _client, cookie_client) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; @@ -234,7 +232,7 @@ async fn test_invalid_authorization_header_format() -> anyhow::Result<()> { let owner_id = cookie_client.as_model().id; let krate = CrateBuilder::new("foo", owner_id).build(&mut conn).await?; - // Create a client with an invalid authorization header (missing "Bearer " prefix) + // Create a client with an invalid authorization header (missing token prefix) let header = "invalid-format".to_string(); let oidc_token_client = MockTokenUser::with_auth_header(header, app); @@ -247,7 +245,7 @@ async fn test_invalid_authorization_header_format() -> anyhow::Result<()> { } #[tokio::test(flavor = "multi_thread")] -async fn test_invalid_token_format() -> anyhow::Result<()> { +async fn test_invalid_bearer_token_format() -> anyhow::Result<()> { let (app, _client, cookie_client) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; @@ -255,14 +253,14 @@ async fn test_invalid_token_format() -> anyhow::Result<()> { let owner_id = cookie_client.as_model().id; let krate = CrateBuilder::new("foo", owner_id).build(&mut conn).await?; - // Create a client with an invalid authorization header (missing "Bearer " prefix) + // Create a client with an invalid authorization header (missing token prefix) let header = "Bearer invalid-token".to_string(); let oidc_token_client = MockTokenUser::with_auth_header(header, app); let pb = PublishBuilder::new(&krate.name, "1.1.0"); let response = oidc_token_client.publish_crate(pb).await; - assert_snapshot!(response.status(), @"403 Forbidden"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Invalid authentication token"}]}"#); + assert_snapshot!(response.status(), @"401 Unauthorized"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"The given API token does not match the format used by crates.io. Tokens generated before 2020-07-14 were generated with an insecure random number generator, and have been revoked. You can generate a new token at https://crates.io/me. For more information please see https://blog.rust-lang.org/2020/07/14/crates-io-security-advisory.html. We apologize for any inconvenience."}]}"#); Ok(()) } @@ -278,8 +276,7 @@ async fn test_non_existent_token() -> anyhow::Result<()> { // Generate a valid token format, but it doesn't exist in the database let (token, _) = generate_token(); - let header = format!("Bearer {}", token); - let oidc_token_client = MockTokenUser::with_auth_header(header, app); + let oidc_token_client = MockTokenUser::with_auth_header(token, app); let pb = PublishBuilder::new(&krate.name, "1.1.0"); let response = oidc_token_client.publish_crate(pb).await; @@ -295,8 +292,7 @@ async fn test_non_existent_token_with_new_crate() -> anyhow::Result<()> { // Generate a valid token format, but it doesn't exist in the database let (token, _) = generate_token(); - let header = format!("Bearer {}", token); - let oidc_token_client = MockTokenUser::with_auth_header(header, app); + let oidc_token_client = MockTokenUser::with_auth_header(token, app); let pb = PublishBuilder::new("foo", "1.0.0"); let response = oidc_token_client.publish_crate(pb).await; @@ -316,8 +312,7 @@ async fn test_token_for_wrong_crate() -> anyhow::Result<()> { let krate = CrateBuilder::new("foo", owner_id).build(&mut conn).await?; let token = new_token(&mut conn, krate.id).await?; - let header = format!("Bearer {}", token); - let oidc_token_client = MockTokenUser::with_auth_header(header, app); + let oidc_token_client = MockTokenUser::with_auth_header(token, app); let krate = CrateBuilder::new("bar", owner_id).build(&mut conn).await?;