From 10b983b8e7076b385f28f9c79cae19882b1db951 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Sat, 20 Feb 2021 00:13:07 +0100 Subject: [PATCH] Add verification of part numbers in CompleteMultipartUpload (WIP #30) --- src/api/api_server.rs | 4 +-- src/api/s3_delete.rs | 30 ++++++++++------------ src/api/s3_put.rs | 59 ++++++++++++++++++++++++++++++++++++++----- src/api/signature.rs | 10 +++++++- 4 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/api/api_server.rs b/src/api/api_server.rs index 484b407a..92a64418 100644 --- a/src/api/api_server.rs +++ b/src/api/api_server.rs @@ -157,7 +157,7 @@ async fn handler_inner(garage: Arc, req: Request) -> Result, req: Request) -> Result { if params.contains_key(&"delete".to_string()) { // DeleteObjects - Ok(handle_delete_objects(garage, bucket, req).await?) + Ok(handle_delete_objects(garage, bucket, req, content_sha256).await?) } else { debug!( "Body: {}", diff --git a/src/api/s3_delete.rs b/src/api/s3_delete.rs index b019987b..91cfbfbe 100644 --- a/src/api/s3_delete.rs +++ b/src/api/s3_delete.rs @@ -10,6 +10,7 @@ use garage_model::object_table::*; use crate::encoding::*; use crate::error::*; +use crate::signature::verify_signed_content; async fn handle_delete_internal( garage: &Garage, @@ -73,8 +74,11 @@ pub async fn handle_delete_objects( garage: Arc, bucket: &str, req: Request, + content_sha256: Option, ) -> Result, Error> { let body = hyper::body::to_bytes(req.into_body()).await?; + verify_signed_content(content_sha256, &body[..])?; + let cmd_xml = roxmltree::Document::parse(&std::str::from_utf8(&body)?)?; let cmd = parse_delete_objects_xml(&cmd_xml).ok_or_bad_request("Invalid delete XML query")?; @@ -131,33 +135,27 @@ struct DeleteObject { key: String, } -fn parse_delete_objects_xml(xml: &roxmltree::Document) -> Result { +fn parse_delete_objects_xml(xml: &roxmltree::Document) -> Option { let mut ret = DeleteRequest { objects: vec![] }; let root = xml.root(); - let delete = root.first_child().ok_or(format!("Delete tag not found"))?; + let delete = root.first_child()?; if !delete.has_tag_name("Delete") { - return Err(format!("Invalid root tag: {:?}", root)); + return None; } for item in delete.children() { if item.has_tag_name("Object") { - if let Some(key) = item.children().find(|e| e.has_tag_name("Key")) { - if let Some(key_str) = key.text() { - ret.objects.push(DeleteObject { - key: key_str.to_string(), - }); - } else { - return Err(format!("No text for key: {:?}", key)); - } - } else { - return Err(format!("No delete key for item: {:?}", item)); - } + let key = item.children().find(|e| e.has_tag_name("Key"))?; + let key_str = key.text()?; + ret.objects.push(DeleteObject { + key: key_str.to_string(), + }); } else { - return Err(format!("Invalid delete item: {:?}", item)); + return None; } } - Ok(ret) + Some(ret) } diff --git a/src/api/s3_put.rs b/src/api/s3_put.rs index 5b5da3d2..4a2af919 100644 --- a/src/api/s3_put.rs +++ b/src/api/s3_put.rs @@ -11,14 +11,15 @@ use garage_table::*; use garage_util::data::*; use garage_util::error::Error as GarageError; -use crate::error::*; use garage_model::block::INLINE_THRESHOLD; use garage_model::block_ref_table::*; use garage_model::garage::Garage; use garage_model::object_table::*; use garage_model::version_table::*; +use crate::error::*; use crate::encoding::*; +use crate::signature::verify_signed_content; pub async fn handle_put( garage: Arc, @@ -416,11 +417,19 @@ pub async fn handle_put_part( pub async fn handle_complete_multipart_upload( garage: Arc, - _req: Request, + req: Request, bucket: &str, key: &str, upload_id: &str, + content_sha256: Option, ) -> Result, Error> { + let body = hyper::body::to_bytes(req.into_body()).await?; + verify_signed_content(content_sha256, &body[..])?; + + let body_xml = roxmltree::Document::parse(&std::str::from_utf8(&body)?)?; + let body_list_of_parts = parse_complete_multpart_upload_body(&body_xml).ok_or_bad_request("Invalid CompleteMultipartUpload XML")?; + debug!("CompleteMultipartUpload list of parts: {:?}", body_list_of_parts); + let version_uuid = decode_upload_id(upload_id)?; let bucket = bucket.to_string(); @@ -450,6 +459,16 @@ pub async fn handle_complete_multipart_upload( _ => unreachable!(), }; + // Check that the list of parts they gave us corresponds to the parts we have here + // TODO: check MD5 sum of all uploaded parts? but that would mean we have to store them somewhere... + let mut parts = version.blocks().iter().map(|x| x.part_number) + .collect::>(); + parts.dedup(); + let same_parts = body_list_of_parts.iter().map(|x| &x.part_number).eq(parts.iter()); + if !same_parts { + return Err(Error::BadRequest(format!("We don't have the same parts"))); + } + // ETag calculation: we produce ETags that have the same form as // those of S3 multipart uploads, but we don't use their actual // calculation for the first part (we use random bytes). This @@ -465,11 +484,6 @@ pub async fn handle_complete_multipart_upload( num_parts ); - // TODO: check that all the parts that they pretend they gave us are indeed there - // TODO: when we read the XML from _req, remember to check the sha256 sum of the payload - // against the signed x-amz-content-sha256 - // TODO: check MD5 sum of all uploaded parts? but that would mean we have to store them somewhere... - let total_size = version .blocks() .iter() @@ -583,3 +597,34 @@ fn decode_upload_id(id: &str) -> Result { uuid.copy_from_slice(&id_bin[..]); Ok(UUID::from(uuid)) } + +#[derive(Debug)] +struct CompleteMultipartUploadPart { + etag: String, + part_number: u64, +} + +fn parse_complete_multpart_upload_body(xml: &roxmltree::Document) -> Option> { + let mut parts = vec![]; + + let root = xml.root(); + let cmu = root.first_child()?; + if !cmu.has_tag_name("CompleteMultipartUpload") { + return None; + } + + for item in cmu.children() { + if item.has_tag_name("Part") { + let etag = item.children().find(|e| e.has_tag_name("ETag"))?.text()?; + let part_number = item.children().find(|e| e.has_tag_name("PartNumber"))?.text()?; + parts.push(CompleteMultipartUploadPart{ + etag: etag.trim_matches('"').to_string(), + part_number: part_number.parse().ok()?, + }); + } else { + return None; + } + } + + Some(parts) +} diff --git a/src/api/signature.rs b/src/api/signature.rs index 0ee47961..a9876462 100644 --- a/src/api/signature.rs +++ b/src/api/signature.rs @@ -6,7 +6,7 @@ use hyper::{Body, Method, Request}; use sha2::{Digest, Sha256}; use garage_table::*; -use garage_util::data::Hash; +use garage_util::data::{hash, Hash}; use garage_model::garage::Garage; use garage_model::key_table::*; @@ -293,3 +293,11 @@ fn canonical_query_string(uri: &hyper::Uri) -> String { "".to_string() } } + +pub fn verify_signed_content(content_sha256: Option, body: &[u8]) -> Result<(), Error> { + let expected_sha256 = content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?; + if expected_sha256 != hash(body) { + return Err(Error::BadRequest(format!("Request content hash does not match signed hash"))); + } + Ok(()) +}