Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a RuntimePlugin/Interceptor to enforce expected content length #3491

Merged
merged 11 commits into from
Mar 26, 2024

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Mar 14, 2024

Motivation and Context

There is a rarely-triggered bug (awslabs/aws-sdk-rust#1079) that can occur when the runtime is dropped between requests. Although this is definitely the wrong thing to do(tm) which should still aim to at least protect the users from bad data in this case.

This adds an interceptor which validates that the body returned is the correct length.

Description

  • Adds an interceptor that computes the actual content length of the body and compares it to the actual length

Testing

  • Integration style test. Note that this is very hard to test using Hyper because Hyper will attempt to mitigate this issue.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@rcoh rcoh marked this pull request as ready for review March 18, 2024 18:43
@rcoh rcoh requested review from a team as code owners March 18, 2024 18:43
@rcoh rcoh force-pushed the length-enforcing-body branch from fcb33cd to dcb0c0a Compare March 18, 2024 18:44
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

#[pin]
body: InnerBody,
expected_length: u64,
remaining_length: i64,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The u64/i64 difference with all the casting seems fragile to me. Why not just do a checked subtraction and error out if it would go below zero? Or, alternatively, change this from "remaining" to "received" and use addition instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea! that lead to a nice simplification

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 🚀

@rcoh rcoh force-pushed the length-enforcing-body branch 2 times, most recently from d3dacb3 to 9be92d9 Compare March 22, 2024 15:12
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@rcoh rcoh enabled auto-merge March 22, 2024 17:29
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@rcoh rcoh force-pushed the length-enforcing-body branch from 88a1301 to 2fad245 Compare March 26, 2024 18:11
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@rcoh rcoh added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 745d48a Mar 26, 2024
41 checks passed
@rcoh rcoh deleted the length-enforcing-body branch March 26, 2024 19:01
github-merge-queue bot pushed a commit that referenced this pull request Mar 26, 2024
Precommit hooks and CI have started failing due to `runtime-versioner`
reporting `aws-config` requires a version bump.

While the most recent change to that crate was made by [this
PR](#3491) only updating
test-related code, it does still require a version bump.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
github-merge-queue bot pushed a commit that referenced this pull request May 13, 2024
## Motivation and Context
This PR re-enables content-length runtime plugin that was previously
disabled due to some erroneous tests. Those
[tests](https://github.com/awslabs/aws-sdk-rust/blob/main/sdk/dynamodb/tests/retries-with-client-rate-limiting.rs#L29)
appear to be fixed.
 
Original PR: #3491
Tracking issue: #3523


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants