Closed shulin-sq closed 1 month ago
lucix-aws
commented
2 months ago What exactly does this fix? I get that Java is doing it but it's still a behavior change for us. Behavior changes, especially around signing, come with risk. An explanation or really a concrete example of why this matters helps us to be sure we're doing the right thing here.
shulin-sq
commented
2 months ago @lucix-aws hello thank you for the response. As per the issue/discussion post I linked, this was a legitimate issue we ran into when trying to perform sigv4a signing with this library.
I liked an explanation from my coworker which I am going to steal: "the reason Connection should not be included in signature calculations is because it is a hop-by-hop header. that means that individual hops between your client and the aws service api (e.g. forward proxies, CDNs, reverse proxies, etc) are expected to drop the header. so even if the header was sent by your client, it wasn't received by the aws server that did signature validation"
I do wish that the java sdk left a bit more documentation around why this header was excluded so that this is easier to understand.
shulin-sq
commented
2 months ago @lucix-aws are there any other concerns?
lucix-aws
commented
2 months ago Yes - your justification is completely sound on spec, and if I was releasing this today I would elect to include this change, but as it stands I still don't understand what this is actually fixing. Based on some rudimentary local testing we don't appear to be setting a Connection header in the SDKs to begin with.
Behavior changes, especially around signing, come with risk
To expound - this change in behavior affects everybody using the v2 SDK or its sigv4 implementation. We have no real way of knowing whether something out there that consumes this software would be broken by the newly unsigned/un-canonicalized header. I would really rather not touch signing in this way unless absolutely necessary.
As it stands if I took this patch I would be assuming the risk without any understanding of the benefit.
lucix-aws
commented
1 month ago shulin-sq
commented
1 month ago Right this header is not added by the sdk but by the client. My guess is that most aws api's have a server that strips the connection header before the request lands in whatever is responsible for validating the signature. If that is not something that is reproducible, I would also appreciate guidance on how to exclude certain http headers from signing.
If it helps, aside from the java sdk, I also see that it is excluded in the javascript sdk https://github.com/aws/aws-sdk-js-v3/blob/ac9ef805060eafa14c87dda8779d3892a749b76f/packages/signature-v4-crt/src/constants.ts#L22
I'm fine with closing this PR. My intention was to share a finding with others if they run into the same issue, however we have already patched this internally.
lucix-aws
commented
1 month ago Closing in favor of supporting actual generic signers (tracked in #2634).
context: https://github.com/aws/aws-sdk-go-v2/discussions/2594
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection
connection is a hop by hop header that is expected to be stripped by the server. It is already excluded in the java sdk and should be excluded here as well.