From e55fa38c9995294edcdf0f7f4f95dc767b343fb5 Mon Sep 17 00:00:00 2001 From: trinity-1686a Date: Tue, 18 Jan 2022 12:22:31 +0100 Subject: [PATCH] Add date verification to presigned urls (#196) fix #96 fix #162 by returning Forbidden instead Bad Request Co-authored-by: Trinity Pointard Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/196 Co-authored-by: trinity-1686a Co-committed-by: trinity-1686a --- src/api/api_server.rs | 3 ++ src/api/error.rs | 6 +-- src/api/s3_copy.rs | 4 +- src/api/s3_list.rs | 2 +- src/api/signature/payload.rs | 81 ++++++++++++++++++++++++++---------- src/model/helper/error.rs | 6 +-- 6 files changed, 67 insertions(+), 35 deletions(-) diff --git a/src/api/api_server.rs b/src/api/api_server.rs index b34030fd..b064ac24 100644 --- a/src/api/api_server.rs +++ b/src/api/api_server.rs @@ -91,6 +91,9 @@ async fn handler( async fn handler_inner(garage: Arc, req: Request) -> Result, Error> { let (api_key, content_sha256) = check_payload_signature(&garage, &req).await?; + let api_key = api_key.ok_or_else(|| { + Error::Forbidden("Garage does not support anonymous access yet".to_string()) + })?; let authority = req .headers() diff --git a/src/api/error.rs b/src/api/error.rs index 955faed1..8602cfdc 100644 --- a/src/api/error.rs +++ b/src/api/error.rs @@ -206,11 +206,7 @@ where fn ok_or_bad_request>(self, reason: M) -> Result { match self { Ok(x) => Ok(x), - Err(e) => Err(Error::BadRequest(format!( - "{}: {}", - reason.as_ref(), - e.to_string() - ))), + Err(e) => Err(Error::BadRequest(format!("{}: {}", reason.as_ref(), e))), } } } diff --git a/src/api/s3_copy.rs b/src/api/s3_copy.rs index f8b3550e..93947b78 100644 --- a/src/api/s3_copy.rs +++ b/src/api/s3_copy.rs @@ -487,7 +487,7 @@ impl CopyPreconditionHeaders { .get("x-amz-copy-source-if-modified-since") .map(|x| x.to_str()) .transpose()? - .map(|x| httpdate::parse_http_date(x)) + .map(httpdate::parse_http_date) .transpose() .ok_or_bad_request("Invalid date in x-amz-copy-source-if-modified-since")?, copy_source_if_none_match: req @@ -505,7 +505,7 @@ impl CopyPreconditionHeaders { .get("x-amz-copy-source-if-unmodified-since") .map(|x| x.to_str()) .transpose()? - .map(|x| httpdate::parse_http_date(x)) + .map(httpdate::parse_http_date) .transpose() .ok_or_bad_request("Invalid date in x-amz-copy-source-if-unmodified-since")?, }) diff --git a/src/api/s3_list.rs b/src/api/s3_list.rs index f991b07d..c3bc6938 100644 --- a/src/api/s3_list.rs +++ b/src/api/s3_list.rs @@ -121,7 +121,7 @@ pub async fn handle_list( key: uriencode_maybe(key, query.common.urlencode_resp), last_modified: s3_xml::Value(msec_to_rfc3339(info.last_modified)), size: s3_xml::IntValue(info.size as i64), - etag: s3_xml::Value(format!("\"{}\"", info.etag.to_string())), + etag: s3_xml::Value(format!("\"{}\"", info.etag)), storage_class: s3_xml::Value("STANDARD".to_string()), }) .collect(), diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index b13819a8..fe6120d3 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -20,7 +20,7 @@ use crate::error::*; pub async fn check_payload_signature( garage: &Garage, request: &Request, -) -> Result<(Key, Option), Error> { +) -> Result<(Option, Option), Error> { let mut headers = HashMap::new(); for (key, val) in request.headers() { headers.insert(key.to_string(), val.to_str()?.to_string()); @@ -34,24 +34,24 @@ pub async fn check_payload_signature( let authorization = if let Some(authorization) = headers.get("authorization") { parse_authorization(authorization, &headers)? + } else if let Some(algorithm) = headers.get("x-amz-algorithm") { + parse_query_authorization(algorithm, &headers)? } else { - parse_query_authorization(&headers)? + let content_sha256 = headers.get("x-amz-content-sha256"); + if let Some(content_sha256) = content_sha256.filter(|c| "UNSIGNED-PAYLOAD" != c.as_str()) { + let sha256 = hex::decode(content_sha256) + .ok() + .and_then(|bytes| Hash::try_from(&bytes)) + .ok_or_bad_request("Invalid content sha256 hash")?; + return Ok((None, Some(sha256))); + } else { + return Ok((None, None)); + } }; - let date = headers - .get("x-amz-date") - .ok_or_bad_request("Missing X-Amz-Date field")?; - let date: NaiveDateTime = - NaiveDateTime::parse_from_str(date, LONG_DATETIME).ok_or_bad_request("Invalid date")?; - let date: DateTime = DateTime::from_utc(date, Utc); - - if Utc::now() - date > Duration::hours(24) { - return Err(Error::BadRequest("Date is too old".to_string())); - } - let scope = format!( "{}/{}/s3/aws4_request", - date.format(SHORT_DATE), + authorization.date.format(SHORT_DATE), garage.config.s3_api.s3_region ); if authorization.scope != scope { @@ -74,10 +74,10 @@ pub async fn check_payload_signature( &authorization.signed_headers, &authorization.content_sha256, ); - let string_to_sign = string_to_sign(&date, &scope, &canonical_request); + let string_to_sign = string_to_sign(&authorization.date, &scope, &canonical_request); let mut hmac = signing_hmac( - &date, + &authorization.date, &key_p.secret_key, &garage.config.s3_api.s3_region, "s3", @@ -104,7 +104,7 @@ pub async fn check_payload_signature( Some(Hash::try_from(&bytes).ok_or_bad_request("Invalid content sha256 hash")?) }; - Ok((key, content_sha256)) + Ok((Some(key), content_sha256)) } struct Authorization { @@ -113,6 +113,7 @@ struct Authorization { signed_headers: String, signature: String, content_sha256: String, + date: DateTime, } fn parse_authorization( @@ -147,6 +148,17 @@ fn parse_authorization( .get("x-amz-content-sha256") .ok_or_bad_request("Missing X-Amz-Content-Sha256 field")?; + let date = headers + .get("x-amz-date") + .ok_or_bad_request("Missing X-Amz-Date field")?; + let date: NaiveDateTime = + NaiveDateTime::parse_from_str(date, LONG_DATETIME).ok_or_bad_request("Invalid date")?; + let date: DateTime = DateTime::from_utc(date, Utc); + + if Utc::now() - date > Duration::hours(24) { + return Err(Error::BadRequest("Date is too old".to_string())); + } + let auth = Authorization { key_id, scope, @@ -159,15 +171,16 @@ fn parse_authorization( .ok_or_bad_request("Could not find Signature in Authorization field")? .to_string(), content_sha256: content_sha256.to_string(), + date, }; Ok(auth) } -fn parse_query_authorization(headers: &HashMap) -> Result { - let algo = headers - .get("x-amz-algorithm") - .ok_or_bad_request("X-Amz-Algorithm not found in query parameters")?; - if algo != "AWS4-HMAC-SHA256" { +fn parse_query_authorization( + algorithm: &str, + headers: &HashMap, +) -> Result { + if algorithm != "AWS4-HMAC-SHA256" { return Err(Error::BadRequest( "Unsupported authorization method".to_string(), )); @@ -188,12 +201,36 @@ fn parse_query_authorization(headers: &HashMap) -> Result 7 * 24 * 3600 { + return Err(Error::BadRequest( + "X-Amz-Exprires may not exceed a week".to_string(), + )); + } + + let date = headers + .get("x-amz-date") + .ok_or_bad_request("Missing X-Amz-Date field")?; + let date: NaiveDateTime = + NaiveDateTime::parse_from_str(date, LONG_DATETIME).ok_or_bad_request("Invalid date")?; + let date: DateTime = DateTime::from_utc(date, Utc); + + if Utc::now() - date > Duration::seconds(duration) { + return Err(Error::BadRequest("Date is too old".to_string())); + } + Ok(Authorization { key_id, scope, signed_headers: signed_headers.to_string(), signature: signature.to_string(), content_sha256: content_sha256.to_string(), + date, }) } diff --git a/src/model/helper/error.rs b/src/model/helper/error.rs index b9b515f3..30b2ba32 100644 --- a/src/model/helper/error.rs +++ b/src/model/helper/error.rs @@ -31,11 +31,7 @@ where fn ok_or_bad_request>(self, reason: M) -> Result { match self { Ok(x) => Ok(x), - Err(e) => Err(Error::BadRequest(format!( - "{}: {}", - reason.as_ref(), - e.to_string() - ))), + Err(e) => Err(Error::BadRequest(format!("{}: {}", reason.as_ref(), e))), } } }