SdkBody::from_dyn() produces a Request with an invalid signature
#749
Comments
spencergilbert
added
bug
|
Hey @spencergilbert, thanks for submitting this issue. We'll add it to our backlog. Adding a minimal example that reproduces the issue would be much appreciated. |
|
ah—I found the issue. When you wrap the body, we lose the ability to return As a sanity check, will you try using |
|
And as an immediate workaround, I would check to see if |
|
Thanks! I'll take a look at those suggestions this week, I've been meaning to make a MRE but I've just been swamped 😭 |
|
Can confirm only wrapping when Sorry for the delay in the MRE, had a lot of going ons outside of work but I'm taking a look at that again. |
|
ah, I see. Yeah that's tricky. If it's any consolation, when Another thing we should consider is handling this on the signing side by attempting to clone() then read the body to the end during signing instead of just giving up. A final option might be doing something like this to enable your dyn body to still provide a hint about bytes: https://github.com/awslabs/smithy-rs/compare/proto-bytes-hint?expand=1 |
|
Clarifying, @rcoh, is that final option something we could implement on our wrapper type or as part of the SDK itself? |
|
We'll need to do something on the SDK side unfortunately |
|
Would an MRE still be useful here? |
|
No, I fully understand what's going on at this point
Just need to figure out the best solution for progress tracking which I
think is a pretty common desire.
…On Wed, Apr 5, 2023, 12:42 PM Spencer Gilbert ***@***.***> wrote:
Would an MRE still be useful here?
—
Reply to this email directly, view it on GitHub
<#749 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADYKZ24JKGQ7342YSGI443W7WOFZANCNFSM6AAAAAAVMRJPOM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hey @spencergilbert, we're PRing a change that should solve your issue. The PR adds a new method to Would you mind checking out the PR and letting me know what you think? I think you could even test it if you set your project to target this version of |
|
Thank you! I'll try and carve out some time to test that soon. |
|
Sorry @Velfi - I was pulled onto a separate project, I'm just coming back to this now. I see the PR was closed without merging, did this go in a different direction or should I test the previous implementation and the PR could/should be revived? |
|
I'm embarrassed to say I don't even remember why I closed the PR. It might have just been housecleaning. We've been working on a new implementation of the client that changes a lot of things and we may make breaking changes to the body. We've also got work on our calendar to change the way that chunked encoding happens so that we can support more use-cases and I expect that work to be a little related to the closed PR. What was your original reason for defining a body callback? Progress reporting? I think we should start by figuring out what we want to be able to do and then considering if we should extend the old |
|
All good - we had been using the callback to accumulate the bytes sent which we then used in our internal telemetry. I included the original callback code below, it's pretty straightforward. struct BodyCaptureCallback {
bytes_sent: usize,
shared_bytes_sent: Arc<AtomicUsize>,
}
impl BodyCaptureCallback {
fn new() -> (Self, Arc<AtomicUsize>) {
let shared_bytes_sent = Arc::new(AtomicUsize::new(0));
(
Self {
bytes_sent: 0,
shared_bytes_sent: Arc::clone(&shared_bytes_sent),
},
shared_bytes_sent,
)
}
}
impl BodyCallback for BodyCaptureCallback {
fn update(&mut self, bytes: &[u8]) -> Result<(), BoxError> {
// This gets called every time a chunk is read from the request body, which includes both static chunks and
// streaming bodies. Just add the chunk's length to our running tally.
self.bytes_sent += bytes.len();
Ok(())
}
fn trailers(&self) -> Result<Option<headers::HeaderMap<headers::HeaderValue>>, BoxError> {
Ok(None)
}
fn make_new(&self) -> Box<dyn BodyCallback> {
// We technically don't use retries within the AWS side of the API clients, but we have to satisfy this trait
// method, because `aws_smithy_http` uses the retry layer from `tower`, which clones the request regardless
// before it even executes the first attempt... so there's no reason not to make it technically correct.
Box::new(Self {
bytes_sent: 0,
shared_bytes_sent: Arc::clone(&self.shared_bytes_sent),
})
}
} |
|
And a long time since I said I'd check... but I patched the crates we used and the proposed PR does appear to fix the issue @Velfi |
Pulled from smithy-lang/smithy-rs#2567
|
oh great—we'll merge that PR. cc @Velfi can you resurrect and merge? Looks like it needs a rebase but should otherwise apply cleanly. |
rcoh
added
the
pending-release
|
clarification here given the |
|
Fix verified: use aws_sdk_s3::primitives::SdkBody;
fn main() {
let body = SdkBody::from("1234");
let body = body.map_preserve_contents(|b|SdkBody::from_body_0_4(b));
assert_eq!(body.bytes(), Some(&b"1234"[..]));
} |
rcoh
removed
the
pending-release
|
Describe the bug
We recently updated our AWS dependencies and had to replace our usage of
with_body_callback(). I followed the suggestion to wrap theSdkBody, as proposed here.However this change causes the body to be an UNSIGNED-PAYLOAD, which is - as far as we can find - not valid for all services. Looking at other SDK's and implementations, it only seems to be used with S3.
Expected Behavior
A request containing a wrapped
SdkBodyto be correctly signed.Current Behavior
InvalidSignatureExceptionOur requests to S3 do seem to correctly signed when using this same code.
Reproduction Steps
I can look to create a sample, but thought raising this now given the differences with other SDKs worthwhile
Possible Solution
No response
Additional Information/Context
https://docs.rs/aws-sig-auth/latest/src/aws_sig_auth/signer.rs.html#196-197
Checking the JS SDK it seems to be specific to services, rather than generally applied.
Version
Environment details (OS name and version, etc.)
Darwin COMP-J44M27KKWV 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64 arm Darwin
Logs
The text was updated successfully, but these errors were encountered: