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 Sigv4A support #2939

Merged
merged 22 commits into from Sep 28, 2023
Merged

add Sigv4A support #2939

merged 22 commits into from Sep 28, 2023

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Aug 22, 2023

Motivation

smithy-rs#1797

Description

This PR adds Sigv4a support along with many tests that were ported from the CRT Sigv4a project. It does not include:

  • An E2E test. We should create one, but doing so requires us to set up MRAP which is a good amount of work.
  • A fuzz test. While I wrote and ran one on my machine, I found the fuzz testing tool to be really difficult to use and it would take some work to include fuzz testing in CI.

Additionally, test Sigv4a is annoying because Sigv4a signing is non-deterministic. Because of this, the tests test signature generation by first signing a request, and then by verifying that signature (This is how the CRT tests this feature).


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

@github-actions
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.

@Velfi
Copy link
Contributor Author

Velfi commented Aug 24, 2023

currently blocked on #2948

@Velfi Velfi added the blocked label Aug 24, 2023
@github-actions
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.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
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.

@Velfi Velfi removed the blocked label Aug 25, 2023
@Velfi Velfi marked this pull request as ready for review August 25, 2023 20:20
@Velfi Velfi requested review from a team as code owners August 25, 2023 20:20
@github-actions
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.

@github-actions
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.

@github-actions
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.

Super awesome that you got this working! Great work. I have a number of questions/suggestions, and it looks like there might have been some merge issues that resulted in deleted tests.

@github-actions
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
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

LGTM! I'll wait to approve until a changelog entry is available for the PR, which I suspect is not so short.

aws/rust-runtime/aws-sigv4/src/http_request.rs Outdated Show resolved Hide resolved
aws/rust-runtime/aws-sigv4/src/http_request.rs Outdated Show resolved Hide resolved
Co-authored-by: ysaito1001 <awsaito@amazon.com>
@github-actions
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.

@github-actions
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.

@github-actions
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.

@github-actions
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.

@github-actions
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.

use fixed date in MRAP signature test
@github-actions
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.

@Velfi
Copy link
Contributor Author

Velfi commented Sep 27, 2023

nice work on this! I think we're really close. I want to make sure some level of EC2 test / canary is tracked and prioritized.

#3006

@@ -27,7 +27,7 @@ hmac = "0.12"
http = { version = "0.2", optional = true }
num-bigint = { version = "0.4", optional = true }
once_cell = "1.8"
p256 = { version = "0.13.2", features = ["ecdsa"], optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

intentional downgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, b/c it would otherwise conflict with the version of digest required by hmac

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

approved — see inline comments, I don't think they're blocking but we should get to the bottom of them at some point.

@rcoh
Copy link
Collaborator

rcoh commented Sep 27, 2023

you need to add the breaking-change label and rerun semver checks

@Velfi Velfi added the breaking-change This will require a breaking change label Sep 28, 2023
@github-actions
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.

@Velfi Velfi added this pull request to the merge queue Sep 28, 2023
Merged via the queue into main with commit bc41828 Sep 28, 2023
40 of 41 checks passed
@Velfi Velfi deleted the zhessler-sigv4a-attempt-2 branch September 28, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change pending-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants