aws_request_signing_filter hash payload by default #15846
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,16 +2,19 @@ | |
|
|
||
| #include "envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.pb.h" | ||
|
|
||
| #include "common/common/hex.h" | ||
| #include "common/crypto/utility.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace HttpFilters { | ||
| namespace AwsRequestSigningFilter { | ||
|
|
||
| FilterConfigImpl::FilterConfigImpl(Extensions::Common::Aws::SignerPtr&& signer, | ||
| const std::string& stats_prefix, Stats::Scope& scope, | ||
| const std::string& host_rewrite, bool use_unsigned_payload) | ||
| : signer_(std::move(signer)), stats_(Filter::generateStats(stats_prefix, scope)), | ||
| host_rewrite_(host_rewrite), use_unsigned_payload_{use_unsigned_payload} {} | ||
|
|
||
| Filter::Filter(const std::shared_ptr<FilterConfig>& config) : config_(config) {} | ||
|
|
||
|
|
@@ -20,29 +23,76 @@ Extensions::Common::Aws::Signer& FilterConfigImpl::signer() { return *signer_; } | |
| FilterStats& FilterConfigImpl::stats() { return stats_; } | ||
|
|
||
| const std::string& FilterConfigImpl::hostRewrite() const { return host_rewrite_; } | ||
| bool FilterConfigImpl::useUnsignedPayload() const { return use_unsigned_payload_; } | ||
|
|
||
| FilterStats Filter::generateStats(const std::string& prefix, Stats::Scope& scope) { | ||
| const std::string final_prefix = prefix + "aws_request_signing."; | ||
| return {ALL_AWS_REQUEST_SIGNING_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; | ||
| } | ||
|
|
||
| Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { | ||
| const auto& host_rewrite = config_->hostRewrite(); | ||
| const bool use_unsigned_payload = config_->useUnsignedPayload(); | ||
|
|
||
| if (!host_rewrite.empty()) { | ||
| headers.setHost(host_rewrite); | ||
| } | ||
|
|
||
| if (!use_unsigned_payload && !end_stream) { | ||
| request_headers_ = &headers; | ||
| return Http::FilterHeadersStatus::StopIteration; | ||
|
Member
There was a problem hiding this comment. Do we stop iterating to avoid tampering the signature? Probably a comment could be helpful here.
Contributor
Author
There was a problem hiding this comment. Hm, I inferred this as necessary because it's what the buffer filter and the aws lambda filter do. I supposed it was necessary to preclude the headers from being sent upstream before the stream ends. I don't mind adding a comment, but the best I can honestly write with my current understanding is "stop iteration is what other filters do when the request is to be buffered." Did you have something more concrete in mind?
Member
There was a problem hiding this comment. My experience with filter flows is limited, but I think you are right: StopIteration is needed to buffer the request. I don't have strong opinion if a comment is needed. Let's leave it as it is. |
||
| } | ||
|
|
||
| try { | ||
| ENVOY_LOG(debug, "aws request signing from decodeHeaders use_unsigned_payload: {}", | ||
| use_unsigned_payload); | ||
| if (use_unsigned_payload) { | ||
| config_->signer().signUnsignedPayload(headers); | ||
| } else { | ||
| config_->signer().signEmptyPayload(headers); | ||
| } | ||
| config_->stats().signing_added_.inc(); | ||
| } catch (const EnvoyException& e) { | ||
| // TODO: sign should not throw to avoid exceptions in the request path | ||
| ENVOY_LOG(debug, "signing failed: {}", e.what()); | ||
| config_->stats().signing_failed_.inc(); | ||
| } | ||
|
|
||
| return Http::FilterHeadersStatus::Continue; | ||
| } | ||
|
|
||
| Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) { | ||
| if (config_->useUnsignedPayload()) { | ||
| return Http::FilterDataStatus::Continue; | ||
| } | ||
|
|
||
| if (!end_stream) { | ||
| return Http::FilterDataStatus::StopIterationAndBuffer; | ||
| } | ||
|
|
||
| decoder_callbacks_->addDecodedData(data, false); | ||
|
Member
There was a problem hiding this comment. Hmm , I think this should happen before returning StopIterationAndBuffer.
Contributor
Author
There was a problem hiding this comment. Based on the docstrings (item 2, reference below) and implementation in other filters, I think I've used this correctly (this line is only reachable when envoy/include/envoy/http/filter.h Lines 335 to 364 in ad4919d
Member
There was a problem hiding this comment. Yeah you are right, I got confused with the case where the data will be changed/replaced (e.g.: compression). |
||
|
|
||
| const Buffer::Instance& decoding_buffer = *decoder_callbacks_->decodingBuffer(); | ||
|
|
||
| auto& hashing_util = Envoy::Common::Crypto::UtilitySingleton::get(); | ||
| const std::string hash = Hex::encode(hashing_util.getSha256Digest(decoding_buffer)); | ||
|
|
||
| try { | ||
| ENVOY_LOG(debug, "aws request signing from decodeData"); | ||
| ASSERT(request_headers_ != nullptr); | ||
| config_->signer().sign(*request_headers_, hash); | ||
|
Member
There was a problem hiding this comment. ASSERT(request_headers_ != nullptr) |
||
| config_->stats().signing_added_.inc(); | ||
|
Member
There was a problem hiding this comment. Hmm should we add a new stat
Contributor
Author
There was a problem hiding this comment. Having separate counters could add fidelity when |
||
| config_->stats().payload_signing_added_.inc(); | ||
| } catch (const EnvoyException& e) { | ||
|
Member
There was a problem hiding this comment. I think we are moving away from exceptions in the serving path, so maybe add a TODO about making sign() unexceptional |
||
| // TODO: sign should not throw to avoid exceptions in the request path | ||
| ENVOY_LOG(debug, "signing failed: {}", e.what()); | ||
| config_->stats().signing_failed_.inc(); | ||
| config_->stats().payload_signing_failed_.inc(); | ||
| } | ||
|
|
||
| return Http::FilterDataStatus::Continue; | ||
| } | ||
|
|
||
| } // namespace AwsRequestSigningFilter | ||
| } // namespace HttpFilters | ||
| } // namespace Extensions | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the default behavior of the filter need to change to buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I touched on this in the original commit message:
The existing behavior of the filter is that the signature is based on
''unless the service name iss3,es, orglacierin which case the literalUNSIGNED-PAYLOADis used. This behavior is not configurable, so I think we should frame the rest of the discussion as whether the existing behavior should be the default going forward. I argue that the default behavior should not be the existing behavior...The current behavior is incompatible with nearly all AWS services. I can't find any evidence that the changes made in #12020 actually made any cases change from failing to passing, since it only added
esandglacierto the list of services which useUNSIGNED-PAYLOADto calculate the content hash, and neither of those services document that they support that signature variant.In the version history I worded the notes giving the benefit of doubt to what the code expressed, despite not believing this to be true:
We can actually search the AWS SDK models to see which services can use
UNSIGNED-PAYLOAD:Of those 4, only
s3is included in the filter's list to use UNSIGNED-PAYLOAD, so the other 3 are in the same boat as all other services - the filter is only compatible with requests without a body.I've used the new stats to verify that the new behavior still signs requests without a body in
decodeHeaders. So, the only type of requests which are known to have worked with the existing behavior and would be treated differently by the new default behavior are requests to S3 with a body (uploads).For requests to S3 with a body, the new filter will buffer and sign the payload by default. The consequence of that change depends on the size of uploads being handled and the buffer configuration - impact could range from negligible, to higher memory usage, to 413 responses being returned. Of course, the old behavior is available for this use case via the new
use_unsigned_payloadconfig setting.I did consider retaining the old behavior for s3, but the filter needs to support signing payloads for s3 because s3 bucket policies can be used to deny requests with unsigned payloads. If the default s3 behavior is to use unsigned payloads, there would need to be a separate config setting like
sign_s3_payloads, which would greatly increase the complexity of evaluating the filter config.It is impossible to know how prevalently the filter may be used to facilitate uploads to s3 and whether the buffer limit would affect those usages. But, I argue that this one case cannot be assumed to be significant enough to justify the default filter behavior being incompatible with all other AWS services; nor could it justify the more confusing config interface and more complex config evaluation. The version history notes clearly state the change and provide instructions on how to enable the old behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for the really thorough answer-- made it really clear.