From 8a5bbc3b0b1c6ab252d0c98950456a9d4cc2e9fe Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Tue, 1 Mar 2022 11:15:16 +0100 Subject: [PATCH] More permissive OPTIONS on S3 API --- src/api/api_server.rs | 2 +- src/api/s3_cors.rs | 64 +++++++++++++++++++++++++++++++------------ src/web/web_server.rs | 4 +-- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/api/api_server.rs b/src/api/api_server.rs index 5ac78bf4..322ea298 100644 --- a/src/api/api_server.rs +++ b/src/api/api_server.rs @@ -116,7 +116,7 @@ async fn handler_inner(garage: Arc, req: Request) -> Result, req: &Request, bucket_name: Option, ) -> Result, Error> { - let bucket = if let Some(bn) = bucket_name { + // FIXME: CORS rules of buckets with local aliases are + // not taken into account. + + // If the bucket name is a global bucket name, + // we try to apply the CORS rules of that bucket. + // If a user has a local bucket name that has + // the same name, its CORS rules won't be applied + // and will be shadowed by the rules of the globally + // existing bucket (but this is inevitable because + // OPTIONS calls are not auhtenticated). + if let Some(bn) = bucket_name { let helper = garage.bucket_helper(); - let bucket_id = helper - .resolve_global_bucket_name(&bn) - .await? - .ok_or(Error::NoSuchBucket)?; - garage - .bucket_table - .get(&EmptyKey, &bucket_id) - .await? - .filter(|b| !b.state.is_deleted()) - .ok_or(Error::NoSuchBucket)? + 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)?; + handle_options_for_bucket(req, &bucket) + } else { + // If there is a bucket name in the request, but that name + // does not correspond to a global alias for a bucket, + // then it's either a non-existing bucket or a local bucket. + // We have no way of knowing, because the request is not + // authenticated and thus we can't resolve local aliases. + // We take the permissive approach of allowing everything, + // because we don't want to prevent web apps that use + // local bucket names from making API calls. + Ok(Response::builder() + .header(ACCESS_CONTROL_ALLOW_ORIGIN, "*") + .header(ACCESS_CONTROL_ALLOW_METHODS, "*") + .status(StatusCode::OK) + .body(Body::empty())?) + } } else { - // The only supported API call that doesn't use a bucket name is ListBuckets, - // which we want to allow in all cases - return Ok(Response::builder() + // If there is no bucket name in the request, + // we are doing a ListBuckets call, which we want to allow + // for all origins. + Ok(Response::builder() .header(ACCESS_CONTROL_ALLOW_ORIGIN, "*") .header(ACCESS_CONTROL_ALLOW_METHODS, "GET") .status(StatusCode::OK) - .body(Body::empty())?); - }; + .body(Body::empty())?) + } +} +pub fn handle_options_for_bucket( + req: &Request, + bucket: &Bucket, +) -> Result, Error> { let origin = req .headers() .get("Origin") diff --git a/src/web/web_server.rs b/src/web/web_server.rs index 15935cba..80d2feb9 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -13,7 +13,7 @@ use crate::error::*; use garage_api::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}; +use garage_api::s3_cors::{add_cors_headers, find_matching_cors_rule, handle_options_for_bucket}; use garage_api::s3_get::{handle_get, handle_head}; use garage_model::garage::Garage; @@ -133,7 +133,7 @@ async fn serve_file(garage: Arc, req: &Request) -> Result handle_options(garage.clone(), req, Some(bucket_name.to_string())).await, + Method::OPTIONS => handle_options_for_bucket(req, &bucket), Method::HEAD => handle_head(garage.clone(), req, bucket_id, &key, None).await, Method::GET => handle_get(garage.clone(), req, bucket_id, &key, None).await, _ => Err(ApiError::BadRequest("HTTP method not supported".into())),