From ea325d78d36d19f59a0849ace1f4567e2b095bd7 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 May 2022 19:18:51 +0200 Subject: [PATCH] More error refactoring --- src/api/admin/api_server.rs | 8 +-- src/api/admin/bucket.rs | 5 +- src/api/admin/error.rs | 43 +++------------ src/api/common_error.rs | 67 ++++++++++++++++++++++- src/api/helpers.rs | 2 +- src/api/k2v/api_server.rs | 28 +++++----- src/api/k2v/batch.rs | 2 +- src/api/k2v/error.rs | 40 +++++--------- src/api/k2v/range.rs | 2 +- src/api/s3/api_server.rs | 28 +++++----- src/api/s3/bucket.rs | 5 +- src/api/s3/copy.rs | 9 ++- src/api/s3/cors.rs | 32 ++++------- src/api/s3/error.rs | 103 +++++++++++------------------------ src/api/s3/list.rs | 12 +++- src/api/s3/post_object.rs | 15 +++-- src/api/s3/router.rs | 2 +- src/api/s3/website.rs | 23 +++----- src/api/signature/error.rs | 22 +------- src/api/signature/payload.rs | 4 +- src/model/helper/bucket.rs | 12 ++-- src/web/web_server.rs | 4 +- 22 files changed, 209 insertions(+), 259 deletions(-) diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs index b2effa62..098a54aa 100644 --- a/src/api/admin/api_server.rs +++ b/src/api/admin/api_server.rs @@ -114,13 +114,9 @@ impl ApiHandler for AdminApiServer { if let Some(h) = expected_auth_header { match req.headers().get("Authorization") { - None => Err(Error::Forbidden( - "Authorization token must be provided".into(), - )), + None => Err(Error::forbidden("Authorization token must be provided")), Some(v) if v.to_str().map(|hv| hv == h).unwrap_or(false) => Ok(()), - _ => Err(Error::Forbidden( - "Invalid authorization token provided".into(), - )), + _ => Err(Error::forbidden("Invalid authorization token provided")), }?; } diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index b226c015..c5518e4e 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -18,6 +18,7 @@ use garage_model::s3::object_table::ObjectFilter; use crate::admin::error::*; use crate::admin::key::ApiBucketKeyPerm; +use crate::common_error::CommonError; use crate::helpers::parse_json_body; pub async fn handle_list_buckets(garage: &Arc) -> Result, Error> { @@ -233,7 +234,7 @@ pub async fn handle_create_bucket( if let Some(alias) = garage.bucket_alias_table.get(&EmptyKey, ga).await? { if alias.state.get().is_some() { - return Err(Error::BucketAlreadyExists); + return Err(CommonError::BucketAlreadyExists.into()); } } } @@ -333,7 +334,7 @@ pub async fn handle_delete_bucket( ) .await?; if !objects.is_empty() { - return Err(Error::BucketNotEmpty); + return Err(CommonError::BucketNotEmpty.into()); } // --- done checking, now commit --- diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs index 1d68dc69..bb35c16b 100644 --- a/src/api/admin/error.rs +++ b/src/api/admin/error.rs @@ -4,9 +4,9 @@ use hyper::{Body, HeaderMap, StatusCode}; use garage_model::helper::error::Error as HelperError; -use crate::generic_server::ApiError; use crate::common_error::CommonError; -pub use crate::common_error::{OkOrBadRequest, OkOrInternalError}; +pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError}; +use crate::generic_server::ApiError; /// Errors of this crate #[derive(Debug, Error)] @@ -16,30 +16,9 @@ pub enum Error { CommonError(CommonError), // Category: cannot process - /// No proper api key was used, or the signature was invalid - #[error(display = "Forbidden: {}", _0)] - Forbidden(String), - /// The API access key does not exist #[error(display = "Access key not found")] NoSuchAccessKey, - - /// The bucket requested don't exists - #[error(display = "Bucket not found")] - NoSuchBucket, - - /// Tried to create a bucket that already exist - #[error(display = "Bucket already exists")] - BucketAlreadyExists, - - /// Tried to delete a non-empty bucket - #[error(display = "Tried to delete a non-empty bucket")] - BucketNotEmpty, - - // Category: bad request - /// Bucket name is not valid according to AWS S3 specs - #[error(display = "Invalid bucket name")] - InvalidBucketName, } impl From for Error @@ -51,14 +30,16 @@ where } } +impl CommonErrorDerivative for Error {} + impl From for Error { fn from(err: HelperError) -> Self { match err { HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), - HelperError::InvalidBucketName(_) => Self::InvalidBucketName, + HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName), + HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket), HelperError::NoSuchAccessKey(_) => Self::NoSuchAccessKey, - HelperError::NoSuchBucket(_) => Self::NoSuchBucket, } } } @@ -68,10 +49,7 @@ impl ApiError for Error { fn http_status_code(&self) -> StatusCode { match self { Error::CommonError(c) => c.http_status_code(), - Error::NoSuchAccessKey | Error::NoSuchBucket => StatusCode::NOT_FOUND, - Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, - Error::Forbidden(_) => StatusCode::FORBIDDEN, - Error::InvalidBucketName => StatusCode::BAD_REQUEST, + Error::NoSuchAccessKey => StatusCode::NOT_FOUND, } } @@ -80,15 +58,10 @@ impl ApiError for Error { } fn http_body(&self, garage_region: &str, path: &str) -> Body { + // TODO nice json error Body::from(format!( "ERROR: {}\n\ngarage region: {}\npath: {}", self, garage_region, path )) } } - -impl Error { - pub fn bad_request(msg: M) -> Self { - Self::CommonError(CommonError::BadRequest(msg.to_string())) - } -} diff --git a/src/api/common_error.rs b/src/api/common_error.rs index 48106e03..b6dbf059 100644 --- a/src/api/common_error.rs +++ b/src/api/common_error.rs @@ -6,7 +6,7 @@ use garage_util::error::Error as GarageError; /// Errors of this crate #[derive(Debug, Error)] pub enum CommonError { - // Category: internal error + // ---- INTERNAL ERRORS ---- /// Error related to deeper parts of Garage #[error(display = "Internal error: {}", _0)] InternalError(#[error(source)] GarageError), @@ -19,9 +19,34 @@ pub enum CommonError { #[error(display = "Internal error (HTTP error): {}", _0)] Http(#[error(source)] http::Error), - /// The client sent an invalid request + // ---- GENERIC CLIENT ERRORS ---- + /// Proper authentication was not provided + #[error(display = "Forbidden: {}", _0)] + Forbidden(String), + + /// Generic bad request response with custom message #[error(display = "Bad request: {}", _0)] BadRequest(String), + + // ---- SPECIFIC ERROR CONDITIONS ---- + // These have to be error codes referenced in the S3 spec here: + // https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList + /// The bucket requested don't exists + #[error(display = "Bucket not found")] + NoSuchBucket, + + /// Tried to create a bucket that already exist + #[error(display = "Bucket already exists")] + BucketAlreadyExists, + + /// Tried to delete a non-empty bucket + #[error(display = "Tried to delete a non-empty bucket")] + BucketNotEmpty, + + // Category: bad request + /// Bucket name is not valid according to AWS S3 specs + #[error(display = "Invalid bucket name")] + InvalidBucketName, } impl CommonError { @@ -36,15 +61,53 @@ impl CommonError { StatusCode::INTERNAL_SERVER_ERROR } CommonError::BadRequest(_) => StatusCode::BAD_REQUEST, + CommonError::Forbidden(_) => StatusCode::FORBIDDEN, + CommonError::NoSuchBucket => StatusCode::NOT_FOUND, + CommonError::BucketNotEmpty | CommonError::BucketAlreadyExists => StatusCode::CONFLICT, + CommonError::InvalidBucketName => StatusCode::BAD_REQUEST, } } + pub fn aws_code(&self) -> &'static str { + match self { + CommonError::Forbidden(_) => "AccessDenied", + CommonError::InternalError( + GarageError::Timeout + | GarageError::RemoteError(_) + | GarageError::Quorum(_, _, _, _), + ) => "ServiceUnavailable", + CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => { + "InternalError" + } + CommonError::BadRequest(_) => "InvalidRequest", + CommonError::NoSuchBucket => "NoSuchBucket", + CommonError::BucketAlreadyExists => "BucketAlreadyExists", + CommonError::BucketNotEmpty => "BucketNotEmpty", + CommonError::InvalidBucketName => "InvalidBucketName", + } + } pub fn bad_request(msg: M) -> Self { CommonError::BadRequest(msg.to_string()) } } +pub trait CommonErrorDerivative: From { + fn internal_error(msg: M) -> Self { + Self::from(CommonError::InternalError(GarageError::Message( + msg.to_string(), + ))) + } + + fn bad_request(msg: M) -> Self { + Self::from(CommonError::BadRequest(msg.to_string())) + } + + fn forbidden(msg: M) -> Self { + Self::from(CommonError::Forbidden(msg.to_string())) + } +} + /// Trait to map error to the Bad Request error code pub trait OkOrBadRequest { type S; diff --git a/src/api/helpers.rs b/src/api/helpers.rs index 599be3f7..aa350e3c 100644 --- a/src/api/helpers.rs +++ b/src/api/helpers.rs @@ -2,7 +2,7 @@ use hyper::{Body, Request}; use idna::domain_to_unicode; use serde::Deserialize; -use crate::common_error::{*, CommonError as Error}; +use crate::common_error::{CommonError as Error, *}; /// What kind of authorization is required to perform a given action #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs index b70fcdff..eb0fbdd7 100644 --- a/src/api/k2v/api_server.rs +++ b/src/api/k2v/api_server.rs @@ -7,13 +7,12 @@ use hyper::{Body, Method, Request, Response}; use opentelemetry::{trace::SpanRef, KeyValue}; -use garage_table::util::*; use garage_util::error::Error as GarageError; use garage_model::garage::Garage; -use crate::k2v::error::*; use crate::generic_server::*; +use crate::k2v::error::*; use crate::signature::payload::check_payload_signature; use crate::signature::streaming::*; @@ -84,14 +83,14 @@ impl ApiHandler for K2VApiServer { // The OPTIONS method is procesed early, before we even check for an API key if let Endpoint::Options = endpoint { - return Ok(handle_options_s3api(garage, &req, Some(bucket_name)).await + return Ok(handle_options_s3api(garage, &req, Some(bucket_name)) + .await .ok_or_bad_request("Error handling OPTIONS")?); } let (api_key, mut content_sha256) = check_payload_signature(&garage, "k2v", &req).await?; - let api_key = api_key.ok_or_else(|| { - Error::Forbidden("Garage does not support anonymous access yet".to_string()) - })?; + let api_key = api_key + .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; let req = parse_streaming_body( &api_key, @@ -101,13 +100,14 @@ impl ApiHandler for K2VApiServer { "k2v", )?; - let bucket_id = garage.bucket_helper().resolve_bucket(&bucket_name, &api_key).await?; + let bucket_id = garage + .bucket_helper() + .resolve_bucket(&bucket_name, &api_key) + .await?; let bucket = garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .filter(|b| !b.state.is_deleted()) - .ok_or(Error::NoSuchBucket)?; + .bucket_helper() + .get_existing_bucket(bucket_id) + .await?; let allowed = match endpoint.authorization_type() { Authorization::Read => api_key.allow_read(&bucket_id), @@ -117,9 +117,7 @@ impl ApiHandler for K2VApiServer { }; if !allowed { - return Err(Error::Forbidden( - "Operation is not allowed for this key.".to_string(), - )); + return Err(Error::forbidden("Operation is not allowed for this key.")); } // Look up what CORS rule might apply to response. diff --git a/src/api/k2v/batch.rs b/src/api/k2v/batch.rs index 8eae471c..db9901cf 100644 --- a/src/api/k2v/batch.rs +++ b/src/api/k2v/batch.rs @@ -12,8 +12,8 @@ use garage_model::garage::Garage; use garage_model::k2v::causality::*; use garage_model::k2v::item_table::*; -use crate::k2v::error::*; use crate::helpers::*; +use crate::k2v::error::*; use crate::k2v::range::read_range; pub async fn handle_insert_batch( diff --git a/src/api/k2v/error.rs b/src/api/k2v/error.rs index 6b9e81e6..4d8c1154 100644 --- a/src/api/k2v/error.rs +++ b/src/api/k2v/error.rs @@ -5,7 +5,7 @@ use hyper::{Body, HeaderMap, StatusCode}; use garage_model::helper::error::Error as HelperError; use crate::common_error::CommonError; -pub use crate::common_error::{OkOrBadRequest, OkOrInternalError}; +pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError}; use crate::generic_server::ApiError; use crate::signature::error::Error as SignatureError; @@ -17,10 +17,6 @@ pub enum Error { CommonError(CommonError), // Category: cannot process - /// No proper api key was used, or the signature was invalid - #[error(display = "Forbidden: {}", _0)] - Forbidden(String), - /// Authorization Header Malformed #[error(display = "Authorization header malformed, expected scope: {}", _0)] AuthorizationHeaderMalformed(String), @@ -29,10 +25,6 @@ pub enum Error { #[error(display = "Key not found")] NoSuchKey, - /// The bucket requested don't exists - #[error(display = "Bucket not found")] - NoSuchBucket, - /// Some base64 encoded data was badly encoded #[error(display = "Invalid base64: {}", _0)] InvalidBase64(#[error(source)] base64::DecodeError), @@ -59,11 +51,15 @@ where } } +impl CommonErrorDerivative for Error {} + impl From for Error { fn from(err: HelperError) -> Self { match err { HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), + HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName), + HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket), e => Self::CommonError(CommonError::BadRequest(format!("{}", e))), } } @@ -73,35 +69,26 @@ impl From for Error { fn from(err: SignatureError) -> Self { match err { SignatureError::CommonError(c) => Self::CommonError(c), - SignatureError::AuthorizationHeaderMalformed(c) => Self::AuthorizationHeaderMalformed(c), - SignatureError::Forbidden(f) => Self::Forbidden(f), + SignatureError::AuthorizationHeaderMalformed(c) => { + Self::AuthorizationHeaderMalformed(c) + } SignatureError::InvalidUtf8Str(i) => Self::InvalidUtf8Str(i), SignatureError::InvalidHeader(h) => Self::InvalidHeader(h), } } } -impl Error { - //pub fn internal_error(msg: M) -> Self { - // Self::CommonError(CommonError::InternalError(GarageError::Message( - // msg.to_string(), - // ))) - //} - - pub fn bad_request(msg: M) -> Self { - Self::CommonError(CommonError::BadRequest(msg.to_string())) - } -} - impl ApiError for Error { /// Get the HTTP status code that best represents the meaning of the error for the client fn http_status_code(&self) -> StatusCode { match self { Error::CommonError(c) => c.http_status_code(), - Error::NoSuchKey | Error::NoSuchBucket => StatusCode::NOT_FOUND, - Error::Forbidden(_) => StatusCode::FORBIDDEN, + Error::NoSuchKey => StatusCode::NOT_FOUND, Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE, - _ => StatusCode::BAD_REQUEST, + Error::AuthorizationHeaderMalformed(_) + | Error::InvalidBase64(_) + | Error::InvalidHeader(_) + | Error::InvalidUtf8Str(_) => StatusCode::BAD_REQUEST, } } @@ -110,6 +97,7 @@ impl ApiError for Error { } fn http_body(&self, garage_region: &str, path: &str) -> Body { + // TODO nice json error Body::from(format!( "ERROR: {}\n\ngarage region: {}\npath: {}", self, garage_region, path diff --git a/src/api/k2v/range.rs b/src/api/k2v/range.rs index 6aa5c90c..1f7dc4cd 100644 --- a/src/api/k2v/range.rs +++ b/src/api/k2v/range.rs @@ -7,8 +7,8 @@ use std::sync::Arc; use garage_table::replication::TableShardedReplication; use garage_table::*; -use crate::k2v::error::*; use crate::helpers::key_after_prefix; +use crate::k2v::error::*; /// Read range in a Garage table. /// Returns (entries, more?, nextStart) diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index 4df9ee6d..87d0f288 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -8,14 +8,13 @@ use hyper::{Body, Method, Request, Response}; use opentelemetry::{trace::SpanRef, KeyValue}; -use garage_table::util::*; use garage_util::error::Error as GarageError; use garage_model::garage::Garage; use garage_model::key_table::Key; -use crate::s3::error::*; use crate::generic_server::*; +use crate::s3::error::*; use crate::signature::payload::check_payload_signature; use crate::signature::streaming::*; @@ -119,14 +118,14 @@ impl ApiHandler for S3ApiServer { return handle_post_object(garage, req, bucket_name.unwrap()).await; } if let Endpoint::Options = endpoint { - return handle_options_s3api(garage, &req, bucket_name).await + return handle_options_s3api(garage, &req, bucket_name) + .await .map_err(Error::from); } let (api_key, mut content_sha256) = check_payload_signature(&garage, "s3", &req).await?; - let api_key = api_key.ok_or_else(|| { - Error::Forbidden("Garage does not support anonymous access yet".to_string()) - })?; + let api_key = api_key + .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; let req = parse_streaming_body( &api_key, @@ -150,13 +149,14 @@ impl ApiHandler for S3ApiServer { return handle_create_bucket(&garage, req, content_sha256, api_key, bucket_name).await; } - let bucket_id = garage.bucket_helper().resolve_bucket(&bucket_name, &api_key).await?; + let bucket_id = garage + .bucket_helper() + .resolve_bucket(&bucket_name, &api_key) + .await?; let bucket = garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .filter(|b| !b.state.is_deleted()) - .ok_or(Error::NoSuchBucket)?; + .bucket_helper() + .get_existing_bucket(bucket_id) + .await?; let allowed = match endpoint.authorization_type() { Authorization::Read => api_key.allow_read(&bucket_id), @@ -166,9 +166,7 @@ impl ApiHandler for S3ApiServer { }; if !allowed { - return Err(Error::Forbidden( - "Operation is not allowed for this key.".to_string(), - )); + return Err(Error::forbidden("Operation is not allowed for this key.")); } // Look up what CORS rule might apply to response. diff --git a/src/api/s3/bucket.rs b/src/api/s3/bucket.rs index d4a6b0cb..1304cc07 100644 --- a/src/api/s3/bucket.rs +++ b/src/api/s3/bucket.rs @@ -14,6 +14,7 @@ use garage_util::crdt::*; use garage_util::data::*; use garage_util::time::*; +use crate::common_error::CommonError; use crate::s3::error::*; use crate::s3::xml as s3_xml; use crate::signature::verify_signed_content; @@ -158,7 +159,7 @@ pub async fn handle_create_bucket( // otherwise return a forbidden error. let kp = api_key.bucket_permissions(&bucket_id); if !(kp.allow_write || kp.allow_owner) { - return Err(Error::BucketAlreadyExists); + return Err(CommonError::BucketAlreadyExists.into()); } } else { // Create the bucket! @@ -239,7 +240,7 @@ pub async fn handle_delete_bucket( ) .await?; if !objects.is_empty() { - return Err(Error::BucketNotEmpty); + return Err(CommonError::BucketNotEmpty.into()); } // --- done checking, now commit --- diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs index 2468678e..0fc16993 100644 --- a/src/api/s3/copy.rs +++ b/src/api/s3/copy.rs @@ -18,8 +18,8 @@ use garage_model::s3::block_ref_table::*; use garage_model::s3::object_table::*; use garage_model::s3::version_table::*; +use crate::helpers::parse_bucket_key; use crate::s3::error::*; -use crate::helpers::{parse_bucket_key}; use crate::s3::put::{decode_upload_id, get_headers}; use crate::s3::xml::{self as s3_xml, xmlns_tag}; @@ -413,10 +413,13 @@ async fn get_copy_source( let copy_source = percent_encoding::percent_decode_str(copy_source).decode_utf8()?; let (source_bucket, source_key) = parse_bucket_key(©_source, None)?; - let source_bucket_id = garage.bucket_helper().resolve_bucket(&source_bucket.to_string(), api_key).await?; + let source_bucket_id = garage + .bucket_helper() + .resolve_bucket(&source_bucket.to_string(), api_key) + .await?; if !api_key.allow_read(&source_bucket_id) { - return Err(Error::Forbidden(format!( + return Err(Error::forbidden(format!( "Reading from bucket {} not allowed for this key", source_bucket ))); diff --git a/src/api/s3/cors.rs b/src/api/s3/cors.rs index 1ad4f2f8..c7273464 100644 --- a/src/api/s3/cors.rs +++ b/src/api/s3/cors.rs @@ -15,7 +15,6 @@ use crate::signature::verify_signed_content; use garage_model::bucket_table::{Bucket, CorsRule as GarageCorsRule}; use garage_model::garage::Garage; -use garage_table::*; use garage_util::data::*; pub async fn handle_get_cors(bucket: &Bucket) -> Result, Error> { @@ -48,14 +47,11 @@ pub async fn handle_delete_cors( bucket_id: Uuid, ) -> Result, Error> { let mut bucket = garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .ok_or(Error::NoSuchBucket)?; + .bucket_helper() + .get_existing_bucket(bucket_id) + .await?; - let param = bucket - .params_mut() - .ok_or_internal_error("Bucket should not be deleted at this point")?; + let param = bucket.params_mut().unwrap(); param.cors_config.update(None); garage.bucket_table.insert(&bucket).await?; @@ -78,14 +74,11 @@ pub async fn handle_put_cors( } let mut bucket = garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .ok_or(Error::NoSuchBucket)?; + .bucket_helper() + .get_existing_bucket(bucket_id) + .await?; - let param = bucket - .params_mut() - .ok_or_internal_error("Bucket should not be deleted at this point")?; + let param = bucket.params_mut().unwrap(); let conf: CorsConfiguration = from_reader(&body as &[u8])?; conf.validate()?; @@ -119,12 +112,7 @@ pub async fn handle_options_s3api( let helper = garage.bucket_helper(); let bucket_id = helper.resolve_global_bucket_name(&bn).await?; if let Some(id) = bucket_id { - let bucket = garage - .bucket_table - .get(&EmptyKey, &id) - .await? - .filter(|b| !b.state.is_deleted()) - .ok_or(Error::NoSuchBucket)?; + let bucket = garage.bucket_helper().get_existing_bucket(id).await?; handle_options_for_bucket(req, &bucket) } else { // If there is a bucket name in the request, but that name @@ -185,7 +173,7 @@ pub fn handle_options_for_bucket( } } - Err(Error::Forbidden("This CORS request is not allowed.".into())) + Err(Error::forbidden("This CORS request is not allowed.")) } pub fn find_matching_cors_rule<'a>( diff --git a/src/api/s3/error.rs b/src/api/s3/error.rs index a0c4703c..4edff3a1 100644 --- a/src/api/s3/error.rs +++ b/src/api/s3/error.rs @@ -5,10 +5,9 @@ use hyper::header::HeaderValue; use hyper::{Body, HeaderMap, StatusCode}; use garage_model::helper::error::Error as HelperError; -use garage_util::error::Error as GarageError; use crate::common_error::CommonError; -pub use crate::common_error::{OkOrBadRequest, OkOrInternalError}; +pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError}; use crate::generic_server::ApiError; use crate::s3::xml as s3_xml; use crate::signature::error::Error as SignatureError; @@ -21,10 +20,6 @@ pub enum Error { CommonError(CommonError), // Category: cannot process - /// No proper api key was used, or the signature was invalid - #[error(display = "Forbidden: {}", _0)] - Forbidden(String), - /// Authorization Header Malformed #[error(display = "Authorization header malformed, expected scope: {}", _0)] AuthorizationHeaderMalformed(String), @@ -33,22 +28,10 @@ pub enum Error { #[error(display = "Key not found")] NoSuchKey, - /// The bucket requested don't exists - #[error(display = "Bucket not found")] - NoSuchBucket, - /// The multipart upload requested don't exists #[error(display = "Upload not found")] NoSuchUpload, - /// Tried to create a bucket that already exist - #[error(display = "Bucket already exists")] - BucketAlreadyExists, - - /// Tried to delete a non-empty bucket - #[error(display = "Tried to delete a non-empty bucket")] - BucketNotEmpty, - /// Precondition failed (e.g. x-amz-copy-source-if-match) #[error(display = "At least one of the preconditions you specified did not hold")] PreconditionFailed, @@ -75,14 +58,6 @@ pub enum Error { #[error(display = "Invalid UTF-8: {}", _0)] InvalidUtf8String(#[error(source)] std::string::FromUtf8Error), - /// Some base64 encoded data was badly encoded - #[error(display = "Invalid base64: {}", _0)] - InvalidBase64(#[error(source)] base64::DecodeError), - - /// Bucket name is not valid according to AWS S3 specs - #[error(display = "Invalid bucket name")] - InvalidBucketName, - /// The client sent invalid XML data #[error(display = "Invalid XML: {}", _0)] InvalidXml(String), @@ -95,10 +70,6 @@ pub enum Error { #[error(display = "Invalid HTTP range: {:?}", _0)] InvalidRange(#[error(from)] (http_range::HttpRangeParseError, u64)), - /// The client asked for an invalid return format (invalid Accept header) - #[error(display = "Not acceptable: {}", _0)] - NotAcceptable(String), - /// The client sent a request for an action not supported by garage #[error(display = "Unimplemented action: {}", _0)] NotImplemented(String), @@ -113,6 +84,20 @@ where } } +impl CommonErrorDerivative for Error {} + +impl From for Error { + fn from(err: HelperError) -> Self { + match err { + HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), + HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), + HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName), + HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket), + e => Self::bad_request(format!("{}", e)), + } + } +} + impl From for Error { fn from(err: roxmltree::Error) -> Self { Self::InvalidXml(format!("{}", err)) @@ -125,22 +110,13 @@ impl From for Error { } } -impl From for Error { - fn from(err: HelperError) -> Self { - match err { - HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), - HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), - e => Self::CommonError(CommonError::BadRequest(format!("{}", e))), - } - } -} - impl From for Error { fn from(err: SignatureError) -> Self { match err { SignatureError::CommonError(c) => Self::CommonError(c), - SignatureError::AuthorizationHeaderMalformed(c) => Self::AuthorizationHeaderMalformed(c), - SignatureError::Forbidden(f) => Self::Forbidden(f), + SignatureError::AuthorizationHeaderMalformed(c) => { + Self::AuthorizationHeaderMalformed(c) + } SignatureError::InvalidUtf8Str(i) => Self::InvalidUtf8Str(i), SignatureError::InvalidHeader(h) => Self::InvalidHeader(h), } @@ -156,39 +132,22 @@ impl From for Error { impl Error { pub fn aws_code(&self) -> &'static str { match self { + Error::CommonError(c) => c.aws_code(), Error::NoSuchKey => "NoSuchKey", - Error::NoSuchBucket => "NoSuchBucket", Error::NoSuchUpload => "NoSuchUpload", - Error::BucketAlreadyExists => "BucketAlreadyExists", - Error::BucketNotEmpty => "BucketNotEmpty", Error::PreconditionFailed => "PreconditionFailed", Error::InvalidPart => "InvalidPart", Error::InvalidPartOrder => "InvalidPartOrder", Error::EntityTooSmall => "EntityTooSmall", - Error::Forbidden(_) => "AccessDenied", Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed", Error::NotImplemented(_) => "NotImplemented", - Error::CommonError(CommonError::InternalError( - GarageError::Timeout - | GarageError::RemoteError(_) - | GarageError::Quorum(_, _, _, _), - )) => "ServiceUnavailable", - Error::CommonError( - CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_), - ) => "InternalError", - _ => "InvalidRequest", + Error::InvalidXml(_) => "MalformedXML", + Error::InvalidRange(_) => "InvalidRange", + Error::InvalidUtf8Str(_) | Error::InvalidUtf8String(_) | Error::InvalidHeader(_) => { + "InvalidRequest" + } } } - - pub fn internal_error(msg: M) -> Self { - Self::CommonError(CommonError::InternalError(GarageError::Message( - msg.to_string(), - ))) - } - - pub fn bad_request(msg: M) -> Self { - Self::CommonError(CommonError::BadRequest(msg.to_string())) - } } impl ApiError for Error { @@ -196,14 +155,18 @@ impl ApiError for Error { fn http_status_code(&self) -> StatusCode { match self { Error::CommonError(c) => c.http_status_code(), - Error::NoSuchKey | Error::NoSuchBucket | Error::NoSuchUpload => StatusCode::NOT_FOUND, - Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, + Error::NoSuchKey | Error::NoSuchUpload => StatusCode::NOT_FOUND, Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED, - Error::Forbidden(_) => StatusCode::FORBIDDEN, - Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE, Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE, Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED, - _ => StatusCode::BAD_REQUEST, + Error::AuthorizationHeaderMalformed(_) + | Error::InvalidPart + | Error::InvalidPartOrder + | Error::EntityTooSmall + | Error::InvalidXml(_) + | Error::InvalidUtf8Str(_) + | Error::InvalidUtf8String(_) + | Error::InvalidHeader(_) => StatusCode::BAD_REQUEST, } } diff --git a/src/api/s3/list.rs b/src/api/s3/list.rs index b4ba5bcd..12f6149d 100644 --- a/src/api/s3/list.rs +++ b/src/api/s3/list.rs @@ -16,8 +16,8 @@ use garage_model::s3::version_table::Version; use garage_table::{EmptyKey, EnumerationOrder}; use crate::encoding::*; -use crate::s3::error::*; use crate::helpers::key_after_prefix; +use crate::s3::error::*; use crate::s3::put as s3_put; use crate::s3::xml as s3_xml; @@ -582,11 +582,17 @@ impl ListObjectsQuery { // representing the key to start with. (Some(token), _) => match &token[..1] { "[" => Ok(RangeBegin::IncludingKey { - key: String::from_utf8(base64::decode(token[1..].as_bytes())?)?, + key: String::from_utf8( + base64::decode(token[1..].as_bytes()) + .ok_or_bad_request("Invalid continuation token")?, + )?, fallback_key: None, }), "]" => Ok(RangeBegin::AfterKey { - key: String::from_utf8(base64::decode(token[1..].as_bytes())?)?, + key: String::from_utf8( + base64::decode(token[1..].as_bytes()) + .ok_or_bad_request("Invalid continuation token")?, + )?, }), _ => Err(Error::bad_request("Invalid continuation token".to_string())), }, diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs index c4b63452..302ebe01 100644 --- a/src/api/s3/post_object.rs +++ b/src/api/s3/post_object.rs @@ -89,9 +89,7 @@ pub async fn handle_post_object( .to_str()?; let credential = params .get("x-amz-credential") - .ok_or_else(|| { - Error::Forbidden("Garage does not support anonymous access yet".to_string()) - })? + .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))? .to_str()?; let policy = params .get("policy") @@ -128,15 +126,16 @@ pub async fn handle_post_object( ) .await?; - let bucket_id = garage.bucket_helper().resolve_bucket(&bucket, &api_key).await?; + let bucket_id = garage + .bucket_helper() + .resolve_bucket(&bucket, &api_key) + .await?; if !api_key.allow_write(&bucket_id) { - return Err(Error::Forbidden( - "Operation is not allowed for this key.".to_string(), - )); + return Err(Error::forbidden("Operation is not allowed for this key.")); } - let decoded_policy = base64::decode(&policy)?; + let decoded_policy = base64::decode(&policy).ok_or_bad_request("Invalid policy")?; let decoded_policy: Policy = serde_json::from_slice(&decoded_policy).ok_or_bad_request("Invalid policy")?; diff --git a/src/api/s3/router.rs b/src/api/s3/router.rs index b12c63a7..0e769558 100644 --- a/src/api/s3/router.rs +++ b/src/api/s3/router.rs @@ -3,9 +3,9 @@ use std::borrow::Cow; use hyper::header::HeaderValue; use hyper::{HeaderMap, Method, Request}; -use crate::s3::error::{Error, OkOrBadRequest}; use crate::helpers::Authorization; use crate::router_macros::{generateQueryParameters, router_match}; +use crate::s3::error::*; router_match! {@func diff --git a/src/api/s3/website.rs b/src/api/s3/website.rs index b2582c4b..133c8327 100644 --- a/src/api/s3/website.rs +++ b/src/api/s3/website.rs @@ -10,7 +10,6 @@ use crate::signature::verify_signed_content; use garage_model::bucket_table::*; use garage_model::garage::Garage; -use garage_table::*; use garage_util::data::*; pub async fn handle_get_website(bucket: &Bucket) -> Result, Error> { @@ -47,14 +46,11 @@ pub async fn handle_delete_website( bucket_id: Uuid, ) -> Result, Error> { let mut bucket = garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .ok_or(Error::NoSuchBucket)?; + .bucket_helper() + .get_existing_bucket(bucket_id) + .await?; - let param = bucket - .params_mut() - .ok_or_internal_error("Bucket should not be deleted at this point")?; + let param = bucket.params_mut().unwrap(); param.website_config.update(None); garage.bucket_table.insert(&bucket).await?; @@ -77,14 +73,11 @@ pub async fn handle_put_website( } let mut bucket = garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .ok_or(Error::NoSuchBucket)?; + .bucket_helper() + .get_existing_bucket(bucket_id) + .await?; - let param = bucket - .params_mut() - .ok_or_internal_error("Bucket should not be deleted at this point")?; + let param = bucket.params_mut().unwrap(); let conf: WebsiteConfiguration = from_reader(&body as &[u8])?; conf.validate()?; diff --git a/src/api/signature/error.rs b/src/api/signature/error.rs index 69f3c6c5..3ef5cdcd 100644 --- a/src/api/signature/error.rs +++ b/src/api/signature/error.rs @@ -1,9 +1,7 @@ use err_derive::Error; -use garage_util::error::Error as GarageError; - use crate::common_error::CommonError; -pub use crate::common_error::{OkOrBadRequest, OkOrInternalError}; +pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError}; /// Errors of this crate #[derive(Debug, Error)] @@ -16,10 +14,6 @@ pub enum Error { #[error(display = "Authorization header malformed, expected scope: {}", _0)] AuthorizationHeaderMalformed(String), - /// No proper api key was used, or the signature was invalid - #[error(display = "Forbidden: {}", _0)] - Forbidden(String), - // Category: bad request /// The request contained an invalid UTF-8 sequence in its path or in other parameters #[error(display = "Invalid UTF-8: {}", _0)] @@ -39,16 +33,4 @@ where } } - -impl Error { - pub fn internal_error(msg: M) -> Self { - Self::CommonError(CommonError::InternalError(GarageError::Message( - msg.to_string(), - ))) - } - - pub fn bad_request(msg: M) -> Self { - Self::CommonError(CommonError::BadRequest(msg.to_string())) - } -} - +impl CommonErrorDerivative for Error {} diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 155a6f94..4c7934e5 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -303,7 +303,7 @@ pub async fn verify_v4( .get(&EmptyKey, &key_id) .await? .filter(|k| !k.state.is_deleted()) - .ok_or_else(|| Error::Forbidden(format!("No such key: {}", &key_id)))?; + .ok_or_else(|| Error::forbidden(format!("No such key: {}", &key_id)))?; let key_p = key.params().unwrap(); let mut hmac = signing_hmac( @@ -316,7 +316,7 @@ pub async fn verify_v4( hmac.update(payload); let our_signature = hex::encode(hmac.finalize().into_bytes()); if signature != our_signature { - return Err(Error::Forbidden("Invalid signature".to_string())); + return Err(Error::forbidden("Invalid signature".to_string())); } Ok(key) diff --git a/src/model/helper/bucket.rs b/src/model/helper/bucket.rs index 2f1c6ae9..734cb40e 100644 --- a/src/model/helper/bucket.rs +++ b/src/model/helper/bucket.rs @@ -6,10 +6,10 @@ use garage_util::time::*; use crate::bucket_alias_table::*; use crate::bucket_table::*; -use crate::key_table::*; use crate::garage::Garage; use crate::helper::error::*; use crate::helper::key::KeyHelper; +use crate::key_table::*; use crate::permission::BucketKeyPerm; pub struct BucketHelper<'a>(pub(crate) &'a Garage); @@ -51,11 +51,7 @@ impl<'a> BucketHelper<'a> { } #[allow(clippy::ptr_arg)] - pub async fn resolve_bucket( - &self, - bucket_name: &String, - api_key: &Key, - ) -> Result { + pub async fn resolve_bucket(&self, bucket_name: &String, api_key: &Key) -> Result { let api_key_params = api_key .state .as_option() @@ -64,8 +60,8 @@ impl<'a> BucketHelper<'a> { if let Some(Some(bucket_id)) = api_key_params.local_aliases.get(bucket_name) { Ok(*bucket_id) } else { - Ok(self. - resolve_global_bucket_name(bucket_name) + Ok(self + .resolve_global_bucket_name(bucket_name) .await? .ok_or_else(|| Error::NoSuchBucket(bucket_name.to_string()))?) } diff --git a/src/web/web_server.rs b/src/web/web_server.rs index dad98dfc..c30d8957 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -18,9 +18,11 @@ use opentelemetry::{ use crate::error::*; -use garage_api::s3::error::{Error as ApiError, OkOrBadRequest, OkOrInternalError}; use garage_api::helpers::{authority_to_host, host_to_bucket}; use garage_api::s3::cors::{add_cors_headers, find_matching_cors_rule, handle_options_for_bucket}; +use garage_api::s3::error::{ + CommonErrorDerivative, Error as ApiError, OkOrBadRequest, OkOrInternalError, +}; use garage_api::s3::get::{handle_get, handle_head}; use garage_model::garage::Garage;