Skip to content

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Jul 25, 2025

Description

Started as an investigation of #4227.

I found that we 1. weren't utilizing most of our test suite (we only used 4 of the test directories from the v4 suite) and 2. that we had a bug in our canonical request implementation where we sort the query keys before encoding instead of after (see https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html#create-canonical-request and the get-vanilla-query-order-encoded test).

This PR does a few things:

  1. Imported the CRT test suite and removed the old ones.
    • The previous test suite we were using is the older format that was in use and floating around before CRT updated it. The newer version of this has standardized the file format and includes both header and query versions of each test.
    • We were already using this format for the sigv4a tests so now all of our tests for v4 and v4a use the same format. It also has a few additional tests we didn't have.
    • Using the same format as CRT (and Kotlin) will make it easier to update the test suite or add new tests
  2. Updates the test utils to be common across v4 and v4a.
  3. Added new test cases we weren't running for both v4 and v4a. Also every test now generally tests both headers and query signature locations (before we were only testing for headers most of the time).
  4. Fixed a bug in canonical request implementation where we sort the query keys before encoding instead of after (found by new test and pointed out by engineer on ARC team as well).
  5. Migrated a few tests double-encode-path and double-url-encode that we used in several places to the newer format (only for headers not query which was missing).
  6. Inlined the iam related test request where it is used
  7. Added a new insert_encoded for QueryWriter to append an already encoded key/value pair and skip encoding.

Testing

  • During the migration I kept both the old and new test suites around. Then removed the old suite after verifying the tests that utilized the same files still passed on the new test suite.

Checklist

  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

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

@aajtodd aajtodd requested review from a team as code owners July 25, 2025 16:41
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

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

Should we add more docs around SIgnableRequest to make it obvious that the uri should be encoded to address the confusion from #4227, or were there going to be more changes for that issue?

@aajtodd
Copy link
Contributor Author

aajtodd commented Jul 25, 2025

Should we add more docs around SIgnableRequest to make it obvious that the uri should be encoded to address the confusion from #4227, or were there going to be more changes for that issue?

I wasn't planning on making additional changes but yeah I can add some additional docs to signable request to indicate it should be a encoded URI.

Copy link

A new generated diff is ready to view.

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.

Awesome updating the test suite!

@aajtodd aajtodd merged commit 3ef75af into main Jul 25, 2025
45 checks passed
@aajtodd aajtodd deleted the fix-signing branch July 25, 2025 20:09
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