http2: Add flag protected checks for frame flood and abuse by upstream servers #13635
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Yan Avlasov <yavlasov@google.com>
source/common/http/codec_client.cc
Outdated
| } else if (isBufferFloodError(status) || isInboundFramesWithEmptyPayloadError(status)) { | ||
| ENVOY_CONN_LOG(debug, "flood detected: {}", *connection_, status.message()); | ||
| close(); | ||
| protocol_error = true; |
There was a problem hiding this comment.
I wonder if some generalization of this error handling code is appropriate. The isCodecProtocolError is very similar to this path. Having to extend this further as we add other error types seems like unnecessary toil and aspirational ASSERTs.
| return ProtocolConstraints::ReleasorProc([]() {}); | ||
| } | ||
| Status trackInboundFrames(const nghttp2_frame_hd*, uint32_t) override { return okStatus(); } | ||
| // TODO(yanavlasov): move to the base class once upstream flood checks are not flag protected. |
There was a problem hiding this comment.
Could you add a reference to envoy.reloadable_features.upstream_http2_flood_checks so it's easier to find as we remove this feature flag? Alternatively, also add this cleanup comment to the cc file.
| @@ -33,6 +33,7 @@ New Features | |||
| * grpc: implemented header value syntax support when defining :ref:`initial metadata <envoy_v3_api_field_config.core.v3.GrpcService.initial_metadata>` for gRPC-based `ext_authz` :ref:`HTTP <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.grpc_service>` and :ref:`network <envoy_v3_api_field_extensions.filters.network.ext_authz.v3.ExtAuthz.grpc_service>` filters, and :ref:`ratelimit <envoy_v3_api_field_config.ratelimit.v3.RateLimitServiceConfig.grpc_service>` filters. | |||
| * hds: added support for delta updates in the :ref:`HealthCheckSpecifier <envoy_v3_api_msg_service.health.v3.HealthCheckSpecifier>`, making only the Endpoints and Health Checkers that changed be reconstructed on receiving a new message, rather than the entire HDS. | |||
| * health_check: added option to use :ref:`no_traffic_healthy_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_healthy_interval>` which allows a different no traffic interval when the host is healthy. | |||
| * http: added frame flood and abuse checks to the upstream HTTP/2 codec. This check is off by default and can be enabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to true. | |||
There was a problem hiding this comment.
Please update API comments now that this is implemented for the upstream direction: https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/core/v3/protocol.proto#L273
test/config/utility.h
Outdated
| @@ -223,9 +223,12 @@ class ConfigHelper { | |||
| // and write it to the lds file. | |||
| void setLds(absl::string_view version_info); | |||
|
|
|||
| // Set limits on pending outbound frames. | |||
| // Set limits on pending downstream outbound frames. | |||
| void setOutboundFramesLimits(uint32_t max_all_frames, uint32_t max_control_frames); | |||
There was a problem hiding this comment.
Please rename to setDownstreamOutboundFramesLimits
There are only about 2 call sites in tests.
test/integration/fake_upstream.h
Outdated
| // Note: that this write bypasses any processing by the upstream codec. | ||
| ABSL_MUST_USE_RESULT | ||
| testing::AssertionResult | ||
| writeConnection(uint32_t index, const std::string& data, bool end_stream = false, |
There was a problem hiding this comment.
optional naming suggestion: rawWriteConnection
adisuissa
reviewed
Oct 21, 2020
| @@ -1109,6 +1109,18 @@ void ConfigHelper::setOutboundFramesLimits(uint32_t max_all_frames, uint32_t max | |||
| } | |||
| } | |||
|
|
|||
| void ConfigHelper::setUpstreamOutboundFramesLimits(uint32_t max_all_frames, | |||
There was a problem hiding this comment.
Similar to @antoniovicente suggestion, can you please rename setOutboundFramesLimits to setDownstreamOutboundFramesLimits.
Signed-off-by: Yan Avlasov <yavlasov@google.com>
|
/lgtm api |
|
/azp help |
Supported commands
See additional documentation. |
|
/azp list |
|
CI/CD Pipelines for this repository: |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Yan Avlasov <yavlasov@google.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit Message:
Add flag protected checks for frame flood and abuse by upstream servers
Additional Description:
This is the starter PR to enable frame flood and abuse checks for upstream codecs. The check is disabled by default and can be enabled via runtime flag
envoy.reloadable_features.upstream_http2_flood_checks. These checks will be enabled by default once all unit and integration tests are implemented.This PR also introduces mechanism for writing arbitrary H/2 frames to the fake upstream connection.
Part of #12281
Risk Level: Low (disabled by default)
Testing: Unit Tests
Docs Changes: Yes (through proto comments)
Release Notes: Yes
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.upstream_http2_flood_checks
Signed-off-by: Yan Avlasov yavlasov@google.com