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

otelaws: Add finalize middleware after instead of before #5975

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jacksehr
Copy link

@jacksehr jacksehr commented Aug 2, 2024

This PR addresses #3368.

Summary

In any presigned URL flow, the AWS SDK skips all Finalize middlewares after the PresignHTTPMiddleware. So, if we move context propagation after the PresignHTTPMiddleware, we will not include context propagation headers in presigned URL requests, which should prevent the reported issue with signatures.

Context

The existing middleware in this package adds propagation headers (e.g. traceparent) to outgoing AWS requests, presumably to be used in things like AWS X-Ray. However, when making requests for presigned URLs, the AWS SDK adds a bunch of middleware of its own to requests. The most important of these is the PresignHTTPMiddleware. This does a lot of things, but for the purposes of this PR/issue, there are two particularly relevant things it does:

Incorporating request headers into the presigned URL signature

When it is built, the middleware receives a presigner. Then, when it is actually run, it receives a *smithyHTTP.Request, and it's entirely up to the presigner if the headers on the *smithyHTTP.Request are incorporated in the calculation of the presigned URL's signature. If we look in the internals of the default v4 signer, it appears to just incorporate all existing headers and use them to sign the URL. This leads to the problem described in the above issue:

The URL generated from code with otelaws middlewares has two values host and traceparent, while the one without otelaws middlewares has only host.

As observed by the comments in that issue, disabling the context propagation solves this issue, which makes sense as that removes the traceparent header from the signature calculation.

This leads us to the second relevant behaviour of the PresignHTTPMiddleware.

Short circuiting remaining middleware

From the comments of the PresignHTTPMiddleware:

// PresignHTTPRequestMiddleware provides the Finalize middleware for creating a
// presigned URL for an HTTP request.
//
// Will short circuit the middleware stack and not forward onto the next
// Finalize handler.

In addition to this, the Deserialize middlewares get cleared, as seen here.

So, in the presigned URL flow, the last middleware that will get called will be the PresignHTTPRequestMiddleware. This is actually useful for us now, as if we just ensure our context propagation middleware comes last in the Finalize step, then we can be sure it will never run in the presign flow. Simply shifting from middleware.Before to middleware.After does just that.

I've also renamed the function and moved it so that its order reflects the order of middleware execution (i.e. finalize before deserialize)

Copy link

linux-foundation-easycla bot commented Aug 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@jacksehr jacksehr marked this pull request as ready for review August 5, 2024 01:00
@jacksehr jacksehr requested a review from a team August 5, 2024 01:00
@pellared pellared changed the title awsotel: add finalize middleware after instead of before otelaws: Add finalize middleware after instead of before Aug 7, 2024
@pellared
Copy link
Member

pellared commented Aug 7, 2024

@akats7 PTAL as a code owner.

@pellared pellared assigned pellared and akats7 and unassigned pellared Aug 7, 2024
@akats7
Copy link
Contributor

akats7 commented Aug 13, 2024

Hey @jacksehr, will take a look at this later today, thanks

@akats7
Copy link
Contributor

akats7 commented Aug 20, 2024

Hey @jacksehr, can you please resolve the issues in the unit tests to use your updated method, also would you be able to add/update a test case to reflect this change.

@akats7
Copy link
Contributor

akats7 commented Sep 12, 2024

Hey @jacksehr, any update on this?

@jacksehr jacksehr requested a review from a team as a code owner November 15, 2024 03:46
@jacksehr
Copy link
Author

Hey @jacksehr, any update on this?

Hey @akats7, sorry for the delay, circling back to this -- I've updated with a test and resolved the compilation issue.

@pellared
Copy link
Member

Please add a changelog entry

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