Disable signing the payload when UnsignedPayload is set to true #3596
Conversation
| { | ||
| // If unsigned payload, set the appropriate magic string in the header and return it | ||
| if (request.DisablePayloadSigning != null ? request.DisablePayloadSigning.Value : !signPayload) | ||
| if (request.UnsignedPayload || request.DisablePayloadSigning.GetValueOrDefault() || !signPayload) |
There was a problem hiding this comment.
GetValueOrDefault will return false for the default value of DisablePayloadSigning. In that case it isn't part of the condition as it was before because before it would use the flag if it was false or true set explicitly and only use !signPayload if it was null. It didn't have to be true. Are you sure this logic is correct?
There is at least one test that sets DisablePayloadSigning to false explicitly and would still go into the if logic before.
There was a problem hiding this comment.
I've updated this one since I did miss it and I didn't think it through, though I think (request.UnsignedPayload || request.DisablePayloadSigning.GetValueOrDefault() || !signPayload) is more appropriate since signPayload comes from the Signer constructor and I don't think it should be overridden if the user did set DisablePayloadSigning to false, but this isn't related to UnsignedPayload change and and keeping the original behavior seems reasonable.
| /// <summary> | ||
| /// Gets and sets a flag that indicates if the payload shouldn't be signed. | ||
| /// </summary> | ||
| bool UnsignedPayload |
There was a problem hiding this comment.
Is there a reason we can't use the existing DisablePayloadSigning property? It's already in IRequest and set in a few places: https://github.com/search?q=repo%3Aaws%2Faws-sdk-net%20DisablePayloadSigning&type=code
There was a problem hiding this comment.
The main reason that I didn't want to use it is that it is documented that it is special for S3 and there is this logic around it that isn't related to the SEP
There was a problem hiding this comment.
We could change the docs to not be S3 specific. Also, perhaps that special logic should be in the SEP if it isn't. Not signing the request and not using https was considered to not have enough validation and the same would apply to any other request if it wasn't S3.
There was a problem hiding this comment.
Agree this should be called out in the SEP (I'd be surprised if we were OK with sending unsigned payloads over plain HTTP...)
I'd vote to keep the existing property and make it generic instead of adding a new one.
There was a problem hiding this comment.
It kinda felt like a breaking change when changing the behavior of DisablePayloadSigning in this way, but since we going to release this in v4 then it should be fine to change it.
muhammad-othman
force-pushed
the
muhamoth/DOTNET-7858-UnsignedPayload-Disable-signing-the-payload
branch
from
15afc77 to
3af208a
Compare
muhammad-othman
force-pushed
the
muhamoth/DOTNET-7858-UnsignedPayload-Disable-signing-the-payload
branch
from
3af208a to
67e69bd
Compare
| { | ||
| "core": { | ||
| "changeLogMessages": [ | ||
| "Disable signing the payload when UnsignedPayload is set to true" |
There was a problem hiding this comment.
It isn't completely clear what UnsignedPayload this is now because the property was removed. Unless this is still referencing the trait in the model. This should probably be clarified:
Disable signing the payload when the operation is modeled as having an UnsignedPayload set to true.
Or
Disable signing the payload when the operation is modeled as having an unsigned payload.
311920a to
494c01f
Compare
muhammad-othman
deleted the
muhamoth/DOTNET-7858-UnsignedPayload-Disable-signing-the-payload
branch
3 participants
Description
As part of SigV4a as a trait SEP, this PR disables signing the payload when
UnsignedPayloadis set to true based on the following paragraph from the SEPMotivation and Context
DOTNET-7858Testing
DRY_RUN-58765492-6dda-400f-bad6-dc4ae269bff4