Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64) #156
No reviewers
lx
Labels
No labels
action
check-aws
action
discussion-needed
action
for-external-contributors
action
for-newcomers
action
more-info-needed
action
need-funding
action
triage-required
kind
correctness
kind
ideas
kind
improvement
kind
performance
kind
testing
kind
usability
kind
wrong-behavior
prio
critical
prio
low
scope
admin-api
scope
background-healing
scope
build
scope
documentation
scope
k8s
scope
layout
scope
metadata
scope
ops
scope
rpc
scope
s3-api
scope
security
scope
telemetry
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#193 Fix Multipart Upload with WinSCP (#164)}
Deuxfleurs/garage
Reference: Deuxfleurs/garage#156
Loading…
Reference in a new issue
No description provided.
Delete branch "KokaKiwi/garage:aws4-payload-signing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #64.
The actual plan would be to add another "chunker" wrapping the received body data to handle the chunked payload.
6b481ef81dto3cfc90f8c63cfc90f8c6to6ca9f2b0e96ca9f2b0e9toed31f42d83ed31f42d83tob884fa006eb884fa006eto7f480a35767f480a3576to064c42c6a0064c42c6a0tof91bc329e3WIP: Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)to Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)506ad8768dto577750418eSupport STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)to WIP: Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)5bc4a47286to79022f918c79022f918cto44dce6dbe9WIP: Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)to Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)Thanks @KokaKiwi for the great work!
Here are some general comments:
We should also support streaming signatures for UploadPart. (and maybe check if any other endpoints are concerned)
We should change the tests to include Minio-client testing without SSL to check that this works. This consists mostly of removing the fake ssl layer that is currently done using socat in the current tests
See also my comments in the code.
@ -81,2 +81,4 @@content_sha256: Option<Hash>,) -> Result<Response<Body>, Error> {let content_sha256 =content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?;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)
@ -271,0 +306,4 @@pub struct Header {pub size: usize,pub signature: Box<[u8]>,Is there a good reason to use
Box<[u8]>instead ofVec<u8>? Or maybe it should be a fixed-size array ? (we have our own type for that:FixedBytes32, akaHash)@ -271,0 +307,4 @@pub struct Header {pub size: usize,pub signature: Box<[u8]>,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 typeHashwhich we use everywhere for 32-byte slices, this should be a good place for it too)@ -271,0 +337,4 @@fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {f.debug_struct("Header").field("size", &self.size).field("signature", &hex::encode(&self.signature))If signature was a
Hashwe would benefit from the customimpl Debug for Hashand we could just derive Debug for Header and not implement it manually@ -271,0 +338,4 @@fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {f.debug_struct("Header").field("size", &self.size).field("signature", &hex::encode(&self.signature))If
signaturewas of typeHashit 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.@ -271,0 +379,4 @@}#[pin_project::pin_project]struct SignedPayloadChunker<S, E>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
@ -271,0 +380,4 @@}#[pin_project::pin_project]struct SignedPayloadChunker<S, E>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
SignedPayloadVerifierorStreamingSignedPayloadVerifier?I think this struct and its associated error type (and also the
payloadsubmodule) should be moved insignature/streaming.rs@ -271,0 +502,4 @@}}struct StreamChunker<S: Stream<Item = Result<Bytes, E>>, E> {Is there a good reason to make this generic on error type E ? Most things in Garage aren't generic and just return an
error::Errorand that's simpler@ -271,0 +504,4 @@}}struct StreamChunker<S: Stream<Item = Result<Bytes, E>>, E> {In the current codebase, nothing is generic on the error type, we always use
garage_api::error::Error. Is there a reason why StreamChunker needs a generic error type?@ -430,2 +670,4 @@content_sha256: Option<Hash>,) -> Result<Response<Body>, Error> {let content_sha256 =content_sha256.ok_or_bad_request("Request content hash not signed, aborting.")?;When doing #164 make sure this doesn't cause more issues, it's the same kind of errors
@ -0,0 +13,4 @@const EMPTY_STRING_HEX_DIGEST: &str ="e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";pub fn check_streaming_payload_signature(This doesn't do a check, it returns the expected signature, so it shouldn't be called
check_*@ -0,0 +39,4 @@.ok_or_internal_error("Unable to build signing HMAC")?;hmac.update(string_to_sign.as_bytes());Hash::try_from(&hmac.finalize().into_bytes()).ok_or_bad_request("Invalid signature")If
hash::try_fromfails 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)@ -0,0 +36,4 @@].join("\n");let mut hmac = signing_hmac(&date, secret_key, &garage.config.s3_api.s3_region, "s3")instead of recalculating the signing key from date, secret_key and region each time, couldn't
check_streaming_payload_signaturetake the precomputed signing key? It would cut down on parameters/fieds here and inSignedPayloadChunker, and be (negligeably) faster due to less hmac computation44dce6dbe9tof7cb811943f7cb811943to1b8ad53b52I think I spotted a logic error, otherwise LGTM. Will merge once we have the smoke test.
@ -0,0 +217,4 @@None => {if this.buf.is_empty() {return Poll::Ready(None);}What if the payload input stream is interrupted in the middle? Do we take care of exiting the infinite loop and return an error?
34c3bac9c2tob2db927077Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)to WIP: Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)WIP: Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)to Support STREAMING-AWS4-HMAC-SHA256-PAYLOAD (#64)@ -0,0 +235,4 @@None => {if this.buf.is_empty() {return Poll::Ready(None);}this bit has an invalid edge case : by cutting the stream just before a new chunk header, an attacker can truncate the file without it being rejected. Getting here (inner stream returns None and this.buf is empy) is either such a truncation, or a call to SignedPayloadStream::poll_next after it returned Ok(Ready(None)) once, which is a contract error (
Ok(Ready(None)) means that the stream has terminated, and poll_next should not be invoked again), so this check and return can be safely removed4c57fb892btob2eda2c13e