Deuxfleurs/garage
48
56
Fork 73
Code Issues 100 Pull requests 13 Releases 30 Wiki Activity

Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64) #156

Merged
lx merged 11 commits from KokaKiwi/garage:aws4-payload-signing into main 2022-01-17 09:55:31 +00:00
Conversation 0 Commits 11 Files changed 13 +208 -65
8 changed files with 208 additions and 65 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 006e5cc231 - Show all commits

6
src/api/api_server.rs
View file

@ -15,7 +15,7 @@ use garage_model::garage::Garage;
use garage_model::key_table::Key;
use crate::error::*;
use crate::signature::check_signature;
use crate::signature::payload::check_payload_signature;
use crate::helpers::*;
use crate::s3_bucket::*;
@ -90,7 +90,7 @@ async fn handler(
}
async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Response<Body>, Error> {
let (api_key, content_sha256) = check_signature(&garage, &req).await?;
let (api_key, content_sha256) = check_payload_signature(&garage, &req).await?;
let authority = req
.headers()
@ -176,7 +176,7 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
.await
}
Endpoint::PutObject { key, .. } => {
handle_put(garage, req, bucket_id, &key, content_sha256).await
handle_put(garage, req, bucket_id, &key, &api_key, content_sha256).await
}
Endpoint::AbortMultipartUpload { key, upload_id, .. } => {
handle_abort_multipart_upload(garage, bucket_id, &key, &upload_id).await

13
src/api/s3_bucket.rs
View file

@ -120,7 +120,10 @@ pub async fn handle_create_bucket(
bucket_name: String,
) -> Result<Response<Body>, Error> {
let body = hyper::body::to_bytes(req.into_body()).await?;
verify_signed_content(content_sha256, &body[..])?;
if let Some(content_sha256) = content_sha256 {
verify_signed_content(content_sha256, &body[..])?;
}
let cmd =
parse_create_bucket_xml(&body[..]).ok_or_bad_request("Invalid create bucket XML query")?;
@ -320,7 +323,7 @@ mod tests {
assert_eq!(
parse_create_bucket_xml(
br#"
<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
</CreateBucketConfiguration >
"#
),
@ -329,8 +332,8 @@ mod tests {
assert_eq!(
parse_create_bucket_xml(
br#"
<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<LocationConstraint>Europe</LocationConstraint>
<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<LocationConstraint>Europe</LocationConstraint>
</CreateBucketConfiguration >
"#
),
@ -339,7 +342,7 @@ mod tests {
assert_eq!(
parse_create_bucket_xml(
br#"
<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
</Crea >
"#
),

3
src/api/s3_delete.rs
View file

@ -80,6 +80,9 @@ pub async fn handle_delete_objects(
req: Request<Body>,
content_sha256: Option<Hash>,
) -> Result<Response<Body>, Error> {
let content_sha256 =
content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?;
KokaKiwi marked this conversation as resolved Outdated
lx commented 2021-12-15 09:50:19 +00:00
Outdated
Review
Copy link

Is it mandatory to provide a signed content-sha256 header? I thought rather it was the opposite because it seems to cause issues on other endpoints (e.g. #164)

Is it mandatory to provide a signed content-sha256 header? I thought rather it was the opposite because it seems to cause issues on other endpoints (e.g. #164)
let body = hyper::body::to_bytes(req.into_body()).await?;
verify_signed_content(content_sha256, &body[..])?;

117
src/api/s3_put.rs
View file

@ -2,6 +2,7 @@ use std::collections::{BTreeMap, VecDeque};
use std::pin::Pin;
use std::sync::Arc;
use chrono::{DateTime, NaiveDateTime, Utc};
use futures::task;
use futures::{prelude::*, TryFutureExt};
use hyper::body::{Body, Bytes};
@ -17,18 +18,21 @@ use garage_util::time::*;
use garage_model::block::INLINE_THRESHOLD;
use garage_model::block_ref_table::*;
use garage_model::garage::Garage;
use garage_model::key_table::Key;
use garage_model::object_table::*;
use garage_model::version_table::*;
use crate::error::*;
use crate::s3_xml;
use crate::signature::verify_signed_content;
use crate::signature::LONG_DATETIME;
use crate::signature::{streaming::check_streaming_payload_signature, verify_signed_content};
pub async fn handle_put(
garage: Arc<Garage>,
req: Request<Body>,
bucket_id: Uuid,
key: &str,
api_key: &Key,
mut content_sha256: Option<Hash>,
) -> Result<Response<Body>, Error> {
// Generate identity of new version
@ -54,11 +58,29 @@ pub async fn handle_put(
};
// Parse body of uploaded file
let body = req.into_body();
let (head, body) = req.into_parts();
let body = match payload_seed_signature {
Some(_) => SignedPayloadChunker::new(body).map_err(Error::from).boxed(),
None => body.map_err(Error::from).boxed(),
let body = if let Some(signature) = payload_seed_signature {
let secret_key = &api_key
.state
.as_option()
.ok_or_internal_error("Deleted key state")?
.secret_key;
let date = head
.headers
.get("x-amz-date")
.ok_or_bad_request("Missing X-Amz-Date field")?
.to_str()?;
let date: NaiveDateTime =
NaiveDateTime::parse_from_str(date, LONG_DATETIME).ok_or_bad_request("Invalid date")?;
let date: DateTime<Utc> = DateTime::from_utc(date, Utc);
SignedPayloadChunker::new(body, garage.clone(), date, secret_key, signature)
.map_err(Error::from)
.boxed()
} else {
body.map_err(Error::from).boxed()
};
let mut chunker = StreamChunker::new(body, garage.config.block_size);
@ -287,7 +309,8 @@ async fn put_block_meta(
}
lx marked this conversation as resolved Outdated
lx commented 2021-12-15 10:01:22 +00:00
Outdated
Review
Copy link

Is there a good reason to use Box<[u8]> instead of Vec<u8>? Or maybe it should be a fixed-size array ? (we have our own type for that: FixedBytes32, aka Hash)

Is there a good reason to use `Box<[u8]>` instead of `Vec<u8>`? Or maybe it should be a fixed-size array ? (we have our own type for that: `FixedBytes32`, aka `Hash`)
KokaKiwi marked this conversation as resolved Outdated
lx commented 2021-12-15 14:18:33 +00:00
Outdated
Review
Copy link

Is there a good reason to use a boxed slice instead of either 1/ a Vec<u8>, or 2/ a constant-sized slice ? (we have the type Hash which we use everywhere for 32-byte slices, this should be a good place for it too)

Is there a good reason to use a boxed slice instead of either 1/ a `Vec<u8>`, or 2/ a constant-sized slice ? (we have the type `Hash` which we use everywhere for 32-byte slices, this should be a good place for it too)
mod payload {
#[derive(Debug)]
use std::fmt;
pub struct Header {
pub size: usize,
pub signature: Box<[u8]>,
@ -295,10 +318,10 @@ mod payload {
impl Header {
pub fn parse(input: &[u8]) -> nom::IResult<&[u8], Self> {
use nom::bytes::complete::tag;
use nom::character::complete::hex_digit1;
use nom::bytes::streaming::tag;
use nom::character::streaming::hex_digit1;
use nom::combinator::map_res;
use nom::number::complete::hex_u32;
use nom::number::streaming::hex_u32;
let (input, size) = hex_u32(input)?;
let (input, _) = tag(";")(input)?;
@ -316,13 +339,29 @@ mod payload {
Ok((input, header))
}
lx marked this conversation as resolved Outdated
lx commented 2021-12-15 10:03:23 +00:00
Outdated
Review
Copy link

If signature was a Hash we would benefit from the custom impl Debug for Hash and we could just derive Debug for Header and not implement it manually

If signature was a `Hash` we would benefit from the custom `impl Debug for Hash` and we could just derive Debug for Header and not implement it manually
}
KokaKiwi marked this conversation as resolved Outdated
lx commented 2021-12-15 14:19:19 +00:00
Outdated
Review
Copy link

If signature was of type Hash it would already have its own debug that shows (part of) the bytes in hex, so we could derive Debug instead of having a custom impl.

If `signature` was of type `Hash` it would already have its own debug that shows (part of) the bytes in hex, so we could derive Debug instead of having a custom impl.
impl fmt::Debug for Header {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Header")
.field("size", &self.size)
.field("signature", &hex::encode(&self.signature))
.finish()
}
}
}
enum SignedPayloadChunkerError<StreamE> {
Stream(StreamE),
InvalidSignature,
Message(String),
}
impl<StreamE> SignedPayloadChunkerError<StreamE> {
fn message(msg: &str) -> Self {
SignedPayloadChunkerError::Message(msg.into())
}
}
impl<StreamE> From<SignedPayloadChunkerError<StreamE>> for Error
where
StreamE: Into<Error>,
@ -330,6 +369,9 @@ where
fn from(err: SignedPayloadChunkerError<StreamE>) -> Self {
match err {
SignedPayloadChunkerError::Stream(e) => e.into(),
SignedPayloadChunkerError::InvalidSignature => {
Error::BadRequest("Invalid payload signature".into())
}
SignedPayloadChunkerError::Message(e) => {
Error::BadRequest(format!("Chunk format error: {}", e))
}
@ -339,7 +381,7 @@ where
impl<E, I> From<nom::error::Error<I>> for SignedPayloadChunkerError<E> {
lx marked this conversation as resolved Outdated
lx commented 2021-12-15 10:08:28 +00:00
Outdated
Review
Copy link

Why is this called a Chunker? As far as I understand, it doesn't give chunks that we control, it just gives a normal stream of (arrays of) bytes

Why is this called a Chunker? As far as I understand, it doesn't give chunks that we control, it just gives a normal stream of (arrays of) bytes
fn from(err: nom::error::Error<I>) -> Self {
KokaKiwi marked this conversation as resolved Outdated
lx commented 2021-12-15 14:20:38 +00:00
Outdated
Review
Copy link

  1. Why is this called a Chunker ? The target here is not to create chunks in a specific way but just to process a stream which happens to be made of byte slices. Shouldn't it rather be called a SignedPayloadVerifier or StreamingSignedPayloadVerifier ?

  2. I think this struct and its associated error type (and also the payload submodule) should be moved in signature/streaming.rs

1. Why is this called a Chunker ? The target here is not to create chunks in a specific way but just to process a stream which happens to be made of byte slices. Shouldn't it rather be called a `SignedPayloadVerifier` or `StreamingSignedPayloadVerifier` ? 2. I think this struct and its associated error type (and also the `payload` submodule) should be moved in `signature/streaming.rs`
Self::Message(err.code.description().into())
Self::message(err.code.description())
}
}
@ -351,16 +393,30 @@ where
#[pin]
stream: S,
buf: bytes::BytesMut,
garage: Arc<Garage>,
datetime: DateTime<Utc>,
secret_key: String,
previous_signature: Hash,
}
impl<S, E> SignedPayloadChunker<S, E>
where
S: Stream<Item = Result<Bytes, E>>,
{
fn new(stream: S) -> Self {
fn new(
stream: S,
garage: Arc<Garage>,
datetime: DateTime<Utc>,
secret_key: &str,
seed_signature: Hash,
) -> Self {
Self {
stream,
buf: bytes::BytesMut::new(),
garage,
datetime,
secret_key: secret_key.into(),
previous_signature: seed_signature,
}
}
}
@ -377,18 +433,18 @@ where
) -> task::Poll<Option<Self::Item>> {
use std::task::Poll;
use nom::bytes::complete::{tag, take};
use nom::bytes::streaming::{tag, take};
let mut this = self.project();
macro_rules! parse_try {
macro_rules! try_parse {
($expr:expr) => {
match $expr {
Ok(value) => value,
Ok(value) => Ok(value),
Err(nom::Err::Incomplete(_)) => continue,
Err(nom::Err::Error(e @ nom::error::Error { .. }))
| Err(nom::Err::Failure(e)) => return Poll::Ready(Some(Err(e.into()))),
}
| Err(nom::Err::Failure(e)) => Err(e),
}?
};
}
@ -409,7 +465,9 @@ where
let input: &[u8] = this.buf;
let (input, header) = parse_try!(payload::Header::parse(input));
let (input, header) = try_parse!(payload::Header::parse(input));
let signature = Hash::try_from(&*header.signature)
.ok_or_else(|| SignedPayloadChunkerError::message("Invalid signature"))?;
// 0-sized chunk is the last
if header.size == 0 {
@ -417,12 +475,30 @@ where
return Poll::Ready(None);
}
let (input, data) = parse_try!(take(header.size)(input));
let (input, _) = parse_try!(tag("\r\n")(input));
let (input, data) = try_parse!(take(header.size)(input));
let (input, _) = try_parse!(tag("\r\n")(input));
let data = Bytes::from(data.to_vec());
let data_sha256sum = sha256sum(&data);
let expected_signature = check_streaming_payload_signature(
this.garage,
this.secret_key,
*this.datetime,
*this.previous_signature,
data_sha256sum,
)
.map_err(|e| {
SignedPayloadChunkerError::Message(format!("Could not build signature: {}", e))
})?;
if signature != expected_signature {
return Poll::Ready(Some(Err(SignedPayloadChunkerError::InvalidSignature)));
}
*this.buf = input.into();
*this.previous_signature = signature;
return Poll::Ready(Some(Ok(data)));
}
}
@ -611,6 +687,9 @@ pub async fn handle_complete_multipart_upload(
upload_id: &str,
content_sha256: Option<Hash>,
) -> Result<Response<Body>, Error> {
let content_sha256 =
content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?;
let body = hyper::body::to_bytes(req.into_body()).await?;
verify_signed_content(content_sha256, &body[..])?;

3
src/api/s3_website.rs
View file

@ -43,6 +43,9 @@ pub async fn handle_put_website(
req: Request<Body>,
content_sha256: Option<Hash>,
) -> Result<Response<Body>, Error> {
let content_sha256 =
content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?;
let body = hyper::body::to_bytes(req.into_body()).await?;
verify_signed_content(content_sha256, &body[..])?;

43
src/api/signature/mod.rs Normal file
View file

@ -0,0 +1,43 @@
use chrono::{DateTime, Utc};
use hmac::{Hmac, Mac, NewMac};
use sha2::Sha256;
use garage_util::data::{sha256sum, Hash};
use crate::error::*;
pub mod payload;
pub mod streaming;
pub const SHORT_DATE: &str = "%Y%m%d";
pub const LONG_DATETIME: &str = "%Y%m%dT%H%M%SZ";
type HmacSha256 = Hmac<Sha256>;
pub fn verify_signed_content(expected_sha256: Hash, body: &[u8]) -> Result<(), Error> {
if expected_sha256 != sha256sum(body) {
return Err(Error::BadRequest(
"Request content hash does not match signed hash".to_string(),
));
}
Ok(())
}
fn signing_hmac(
datetime: &DateTime<Utc>,
secret_key: &str,
region: &str,
service: &str,
) -> Result<HmacSha256, crypto_mac::InvalidKeyLength> {
let secret = String::from("AWS4") + secret_key;
let mut date_hmac = HmacSha256::new_varkey(secret.as_bytes())?;
date_hmac.update(datetime.format(SHORT_DATE).to_string().as_bytes());
let mut region_hmac = HmacSha256::new_varkey(&date_hmac.finalize().into_bytes())?;
region_hmac.update(region.as_bytes());
let mut service_hmac = HmacSha256::new_varkey(&region_hmac.finalize().into_bytes())?;
service_hmac.update(service.as_bytes());
let mut signing_hmac = HmacSha256::new_varkey(&service_hmac.finalize().into_bytes())?;
signing_hmac.update(b"aws4_request");
let hmac = HmacSha256::new_varkey(&signing_hmac.finalize().into_bytes())?;
Ok(hmac)
}

44
src/api/signature.rs → src/api/signature/payload.rs
View file

@ -1,25 +1,23 @@
use std::collections::HashMap;
use chrono::{DateTime, Duration, NaiveDateTime, Utc};
use hmac::{Hmac, Mac, NewMac};
use hmac::Mac;
use hyper::{Body, Method, Request};
use sha2::{Digest, Sha256};
use garage_table::*;
use garage_util::data::{sha256sum, Hash};
use garage_util::data::Hash;
use garage_model::garage::Garage;
use garage_model::key_table::*;
use super::signing_hmac;
use super::{LONG_DATETIME, SHORT_DATE};
use crate::encoding::uri_encode;
use crate::error::*;
const SHORT_DATE: &str = "%Y%m%d";
const LONG_DATETIME: &str = "%Y%m%dT%H%M%SZ";
type HmacSha256 = Hmac<Sha256>;
pub async fn check_signature(
pub async fn check_payload_signature(
garage: &Garage,
request: &Request<Body>,
) -> Result<(Key, Option<Hash>), Error> {
@ -222,25 +220,6 @@ fn string_to_sign(datetime: &DateTime<Utc>, scope_string: &str, canonical_req: &
.join("\n")
}
fn signing_hmac(
datetime: &DateTime<Utc>,
secret_key: &str,
region: &str,
service: &str,
) -> Result<HmacSha256, crypto_mac::InvalidKeyLength> {
let secret = String::from("AWS4") + secret_key;
let mut date_hmac = HmacSha256::new_varkey(secret.as_bytes())?;
date_hmac.update(datetime.format(SHORT_DATE).to_string().as_bytes());
let mut region_hmac = HmacSha256::new_varkey(&date_hmac.finalize().into_bytes())?;
region_hmac.update(region.as_bytes());
let mut service_hmac = HmacSha256::new_varkey(&region_hmac.finalize().into_bytes())?;
service_hmac.update(service.as_bytes());
let mut signing_hmac = HmacSha256::new_varkey(&service_hmac.finalize().into_bytes())?;
signing_hmac.update(b"aws4_request");
let hmac = HmacSha256::new_varkey(&signing_hmac.finalize().into_bytes())?;
Ok(hmac)
}
fn canonical_request(
method: &Method,
url_path: &str,
@ -288,14 +267,3 @@ fn canonical_query_string(uri: &hyper::Uri) -> String {
"".to_string()
}
}
pub fn verify_signed_content(content_sha256: Option<Hash>, body: &[u8]) -> Result<(), Error> {
let expected_sha256 =
content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?;
if expected_sha256 != sha256sum(body) {
return Err(Error::BadRequest(
"Request content hash does not match signed hash".to_string(),
));
}
Ok(())
}

44
src/api/signature/streaming.rs Normal file
View file

@ -0,0 +1,44 @@
use chrono::{DateTime, Utc};
use garage_model::garage::Garage;
use garage_util::data::Hash;
use hmac::Mac;
use super::signing_hmac;
use super::{LONG_DATETIME, SHORT_DATE};
use crate::error::*;
/// Result of `sha256("")`
const EMPTY_STRING_HEX_DIGEST: &str =
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
pub fn check_streaming_payload_signature(
KokaKiwi marked this conversation as resolved Outdated
lx commented 2021-12-15 14:31:01 +00:00
Outdated
Review
Copy link

This doesn't do a check, it returns the expected signature, so it shouldn't be called check_*

This doesn't do a check, it returns the expected signature, so it shouldn't be called `check_*`
garage: &Garage,
secret_key: &str,
date: DateTime<Utc>,
previous_signature: Hash,
content_sha256: Hash,
) -> Result<Hash, Error> {
let scope = format!(
"{}/{}/s3/aws4_request",
date.format(SHORT_DATE),
garage.config.s3_api.s3_region
);
let string_to_sign = [
"AWS4-HMAC-SHA256-PAYLOAD",
&date.format(LONG_DATETIME).to_string(),
&scope,
&hex::encode(previous_signature),
EMPTY_STRING_HEX_DIGEST,
&hex::encode(content_sha256),
]
.join("\n");
let mut hmac = signing_hmac(&date, secret_key, &garage.config.s3_api.s3_region, "s3")
KokaKiwi marked this conversation as resolved Outdated
trinity-1686a commented 2021-12-15 14:53:19 +00:00
Outdated
Review
Copy link

instead of recalculating the signing key from date, secret_key and region each time, couldn't check_streaming_payload_signature take the precomputed signing key? It would cut down on parameters/fieds here and in SignedPayloadChunker, and be (negligeably) faster due to less hmac computation

instead of recalculating the signing key from date, secret_key and region each time, couldn't `check_streaming_payload_signature` take the precomputed signing key? It would cut down on parameters/fieds here and in `SignedPayloadChunker`, and be (negligeably) faster due to less hmac computation
.ok_or_internal_error("Unable to build signing HMAC")?;
hmac.update(string_to_sign.as_bytes());
KokaKiwi marked this conversation as resolved Outdated
lx commented 2021-12-15 09:54:43 +00:00
Outdated
Review
Copy link

If hash::try_from fails here, I think it's an internal error and not a bad request (it can only happen if the hmac doesn't have the good number of bytes, which should never happen)

If `hash::try_from` fails here, I think it's an internal error and not a bad request (it can only happen if the hmac doesn't have the good number of bytes, which should never happen)
👍 1
Hash::try_from(&hmac.finalize().into_bytes()).ok_or_bad_request("Invalid signature")
}