Add option for :whitelist_headers in V4 signer

[cjyclaire]

Feature request
Issue related: #1051

cc:\ @awood45 @trevorrowe

[awood45]

I might be missing something, but why not just remove this whole block and use:

@blacklist = BLACKLIST_HEADERS - whitelist_headers

You already set whitelist headers to be an array by default, so once we get to this point, I'm not sure the if-else block is doing anything.

[cjyclaire]

That's true, I was thinking checking the intersection of the 2 arrays before getting the result. It seems redundant, I'll remove that.

[awood45]

The inner implementation appears to be reasonable, but how would an end user use this? I don't believe we have users creating signer classes directly, so we would need to wire this up to configuration in some way.

[awood45]

Some other notes after a deeper dive:

• In the Aws::Signers::V4 class, this will need to work in the sign method, and the whitelisted headers will have to plumb down to the signed_headers method.
• This does not necessarily involve the instantiation of a new signer instance, in fact, it usually does not.
• You may be changing how the internal interface works, so after we get this done we may want to look at if this is a minor version bump.

[cjyclaire]

This PR is updated and ready for another review.
@trevorrowe , @awood45

[trevorrowe]

I've done a quick review of this. I think we should discuss the method in which white-listed headers are provided. I'm of the opinion that this should not be done as client configuration options. Given the client will never consume these options, they increase the surface area of a client just to act as a convenient bag for grouping options. I suspect this was suggested because a Client instance is passed to the pre-signer. Retrospectively, the Aws::S3::Presigner should not have ever accepted a client instance. Instead it should have accepted a credential provider and a region. We can not change that interface now, but we can deprecate it. Lets do a more in-depth review in person and we can update the pull-request.

[cjyclaire]

This PR is updated and ready for another review @awood45 @trevorrowe

[awood45]

This looks good. We should consider adding an integration test for this to ensure it works the way we intend, for example, a test where we presign a URL, call it with the header we whitelisted set, and see that it responds successfully.

[cjyclaire]

@awood45 Sounds good, make sense. I'll add that : )

[cjyclaire]

PR #1477 just opened includes this feature, we'll focus on that one : )
Closing, feel free to chime in and add comments in that PR : )