http2: refactor flood checks into a separate class. #13098

yanavlasov · 2020-09-15T00:52:25Z

Commit Message:
http2: refactor flood checks into a separate class.

Additional Description:
This change is a prerequisite for addressing flood checks outside of the dispatch context and later extending them to the upstream servers as well. The change should have no functional impact.

Risk Level: Medium (codec refactor)
Testing: Unit, integration tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Yan Avlasov yavlasov@google.com

Signed-off-by: Yan Avlasov <yavlasov@google.com>

antoniovicente · 2020-09-15T18:08:45Z

/assign @antoniovicente

antoniovicente · 2020-09-16T00:42:42Z

source/common/http/http2/codec_impl_legacy.cc

-  if (outbound_control_frames_ > max_outbound_control_frames_ && dispatching_downstream_data_) {
-    stats_.outbound_control_flood_.inc();
-    throw FrameFloodException("Too many control frames in the outbound queue.");
+  auto result = protocol_constraints_.status();


Should we skip the constraints status check if dispatching_downstream_data_ is false?

Also, is there a chance that status flips due to upstream data and then the next downstream data update triggers the error condition?

Refactored.
Yes the upstream data can flip status and next downstream data read will trigger connection error. This is how DATA and HEADER flood checks work, see for instance these tests. This is about to change though in the next PR where upstream protocol violations will cause connection to be disconnected by scheduling a callback.

antoniovicente · 2020-09-16T00:50:39Z

source/common/http/http2/codec_impl_legacy.cc

-  if (outbound_control_frames_ > max_outbound_control_frames_ && dispatching_downstream_data_) {
-    stats_.outbound_control_flood_.inc();
-    throw FrameFloodException("Too many control frames in the outbound queue.");
+  auto result = protocol_constraints_.status();


I was expecting a call to protocol_constraints_.checkOutboundQueueLimits(); in here, but that's not what I'm seeing. Would it make sense to call checkOutboundQueueLimits or rename this method?

Renamed method

antoniovicente · 2020-09-16T00:54:46Z

source/common/http/http2/protocol_constraints.cc

+  releaseOutboundFrame();
+}
+
+Status ProtocolConstraints::checkOutboundQueueLimits() {


dispatching_downstream_data_ is no longer considered in this computation. Is that a problem?

It is still checked in the codec class. I wanted the constraints checker to be free of that detail. The dispatching_downstream_data_ is going away soon when flood checks are applied to upstream servers too.

antoniovicente · 2020-09-16T01:01:08Z

source/common/http/http2/protocol_constraints.cc

+
+Status ProtocolConstraints::checkOutboundQueueLimits() {
+  if (outbound_frames_ > max_outbound_frames_) {
+    stats_.outbound_flood_.inc();


I think you're over counting these stats since you end up calling checkOutboundQueueLimits() from the increment method in cases where you're outputting to upstream instead of downstream.

Possible solution: remove status_ and call checkOutboundQueueLimits() from the downstream connection's checkOutboundQueueLimits() method.

The check* methods are not called if the status has flipped. I have moved status check into the methods to make more readable. Also tests verify stats counts to make sure there is no overcounting.

The comment is related processing in the upstream direction. What prevents outbound flooding in the upstream direction from triggering and incrementing this stat? I think we agree that it is possible to call ProtocolConstraints::checkOutboundQueueLimits in the upstream direction, and the overflow condition can happen in that case.

Yes, it was a problem for outbound frame checks. Should be fixed now.

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov · 2020-09-17T02:57:41Z

#12279

antoniovicente

Sorry if I'm misunderstanding something. It seems that some of the changes you're doing are leaking into ClientConnectionImpl which handles the upstream direction.

antoniovicente · 2020-09-17T05:31:13Z

source/common/http/http2/codec_impl.cc

-           outbound_control_frames_ <= max_outbound_control_frames_);
+    // Protocol constrain violations should set the nghttp2_callback_status_ error, and return at
+    // the statement above.
+    ASSERT(protocol_constraints_.status().ok());


Is it possible to fail this ASSERT in the upstream direction? Should you be calling ASSERT(checkProtocolConstraintsStatus()); instead?

It should not be anymore. Both inbound and outbound frame limits are only checked in the server codec now.

Should not be possible now, since the flood checks that can flip status are only done by the server codec.

antoniovicente · 2020-09-17T05:39:42Z

source/common/http/http2/protocol_constraints.cc

+
+Status ProtocolConstraints::checkOutboundQueueLimits() {
+  if (outbound_frames_ > max_outbound_frames_) {
+    stats_.outbound_flood_.inc();


The comment is related processing in the upstream direction. What prevents outbound flooding in the upstream direction from triggering and incrementing this stat? I think we agree that it is possible to call ProtocolConstraints::checkOutboundQueueLimits in the upstream direction, and the overflow condition can happen in that case.

antoniovicente · 2020-09-17T05:40:49Z

source/common/http/http2/protocol_constraints.cc

+    break;
+  }
+
+  status_.Update(checkInboundFrameLimits());


We seem to be using status_ for both the inbound and outbound directions. Is that a problem?

This is by design. The idea is that only the first violation is recorded and the connection is disconnected at that point. In this case unwinding a deep stack and possibly triggering other errors would not mask the first problem.

antoniovicente · 2020-09-17T05:45:41Z

source/common/http/http2/codec_impl.cc

-  RETURN_IF_ERROR(incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame));
+  auto releasor =
+      protocol_constraints_.incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame);
+  RETURN_IF_ERROR(checkProtocolConstraintsStatus());


Should the call to incrementOutboundFrameCount be done by a ServerConnectionImpl override, and have the ClientConnectionImpl version be a no-op at least until a ClientConnectionImpl version is fully implemented?

Yes, it was a premature change on my part.

yanavlasov · 2020-09-17T15:52:29Z

Yes, it does look like I have leaked the check into the client connection as well. I will fix it.

Signed-off-by: Yan Avlasov <yavlasov@google.com>

antoniovicente

Looks good otherwise. It's great to see progress towards generalizing these security checks.

antoniovicente · 2020-09-18T01:18:30Z

source/common/http/conn_manager_impl.cc

@@ -281,7 +281,7 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool
    const Status status = codec_->dispatch(data);

    ASSERT(!isPrematureResponseError(status));
-    if (isBufferFloodError(status)) {
+    if (isBufferFloodError(status) || isInboundFramesWithEmptyPayloadError(status)) {


Would it be appropriate to ASSERT(status.ok()); on line 292, after the end of this if block?

Question is related to the ASSERT(!isPrematureResponseError(status)); on the previous line, is there a danger of us ignoring some of error status that is added in the future? Granted, this seems like a pre-existing issue.

Yes, I think it is a good suggestion. Fixed

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov added 2 commits September 14, 2020 20:25

http2: refactor flood checks into a separate class.

6e9b760

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Add comments

c18ae70

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov marked this pull request as ready for review September 15, 2020 00:52

yanavlasov requested a review from asraa September 15, 2020 02:45

Fix uninitialized variable in test

763593d

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot assigned antoniovicente Sep 15, 2020

antoniovicente reviewed Sep 16, 2020

View reviewed changes

Address comments

f797f9a

Signed-off-by: Yan Avlasov <yavlasov@google.com>

antoniovicente reviewed Sep 17, 2020

View reviewed changes

yanavlasov added 2 commits September 17, 2020 13:48

Address comments

65fb18d

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Merge branch 'master' into flood-disconnect

b5eaf30

Signed-off-by: Yan Avlasov <yavlasov@google.com>

antoniovicente reviewed Sep 18, 2020

View reviewed changes

Address comments

a15fbe4

Signed-off-by: Yan Avlasov <yavlasov@google.com>

antoniovicente approved these changes Sep 18, 2020

View reviewed changes

yanavlasov merged commit 68967b7 into envoyproxy:master Sep 18, 2020

yanavlasov deleted the flood-disconnect branch October 3, 2020 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: refactor flood checks into a separate class. #13098

http2: refactor flood checks into a separate class. #13098

yanavlasov commented Sep 15, 2020

antoniovicente commented Sep 15, 2020

antoniovicente Sep 16, 2020

yanavlasov Sep 16, 2020 •

edited

Loading

antoniovicente Sep 16, 2020

yanavlasov Sep 16, 2020

yanavlasov Sep 17, 2020

antoniovicente Sep 16, 2020

yanavlasov Sep 16, 2020

antoniovicente Sep 16, 2020

yanavlasov Sep 16, 2020

antoniovicente Sep 17, 2020

yanavlasov Sep 17, 2020

yanavlasov commented Sep 17, 2020

antoniovicente left a comment

antoniovicente Sep 17, 2020

yanavlasov Sep 17, 2020

yanavlasov Sep 17, 2020

antoniovicente Sep 17, 2020

antoniovicente Sep 17, 2020

yanavlasov Sep 17, 2020

antoniovicente Sep 17, 2020

yanavlasov Sep 17, 2020

yanavlasov commented Sep 17, 2020

antoniovicente left a comment

antoniovicente Sep 18, 2020

yanavlasov Sep 18, 2020 •

edited

Loading

http2: refactor flood checks into a separate class. #13098

http2: refactor flood checks into a separate class. #13098

Conversation

yanavlasov commented Sep 15, 2020

antoniovicente commented Sep 15, 2020

Choose a reason for hiding this comment

yanavlasov Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Sep 17, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Sep 17, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

yanavlasov Sep 16, 2020 •

edited

Loading

yanavlasov Sep 18, 2020 •

edited

Loading