HTTP/2 rapid reset mitigation #344

pjfanning · 2023-10-29T14:53:48Z

CVE-2023-44487 #332
adds a throttle and the connection is closed if the reset frame rate is too high
code is largely based on suggestions from @jrudolph
the mima filter had to be added due to the new fields in Http2Settings
the mima check did not work so I copied over the working MiMa.scala from incubator-pekko repo

jrudolph · 2023-10-30T08:30:44Z

Thanks for setting that up and sorry for dropping the ball here.

pjfanning · 2023-11-13T12:02:39Z

@jrudolph @raboof @nvollmar @kerr @gmethvin @mdedetrich would any of you have time to review this? This is a reaction to the Akka team issuing a security warning. I would favour not getting a CVE or attaching our name to the existing CVE-2023-44487 but I'm not against it either. My priority is just to get out a release with some change.

project/MiMa.scala

He-Pin · 2023-11-14T11:36:14Z

http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala

+      case _: FrameEvent.DataFrame         => 0
+      case _: FrameEvent.WindowUpdateFrame => 0 // TODO: should we throttle these?
+      case _                               => 1
+    }


this can be contantFunc

I don't know what constantFunc means. Do you mean making this a val instead of a def?

Yes, make just like the Keep.left/right

Is 7156492 the sort of change that you want?

http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala

http-core/src/main/resources/reference.conf

mdedetrich

In terms of implementation I don't have too many issues however @He-Pin if you have time it would be great to address @pjfanning 's outstanding issues.

The MiMa changes definitely should go into a separate PR (which can be merged before this one at which point this PR should be rebased).

Also at the risk of bikeshedding, I would prefer if the resets-throttle-cost config was changed to reset-throttle-cost (i.e. de-pluralize the resets). To me as an english speaker "resets" sounds slightly off. This is however not a deal breaker from my side.

project/MiMa.scala

author PJ Fanning <[email protected]> 1698583210 +0000 committer PJ Fanning <[email protected]> 1702470760 +0100 test for http2 rapid reset Co-Authored-By: Johannes Rudolph <[email protected]> Update Http2ServerSpec.scala add throttle and config Co-Authored-By: Johannes Rudolph <[email protected]> revert code format change Update Http2ServerSettings.scala Update Http2ServerSpec.scala rework test - still needs proper asserts refactor tests scalafmt Create http2-rapid-reset-configs.backwards.excludes refactor test update test add ability to disable throttle Update Http2ServerDisableResetSpec.scala use keepLeft after rapidResetMitigation Update http2-rapid-reset-configs.backwards.excludes uptake sbt-pekko-build use updated plugin refactor sbt-pekko-build 0.1.0 rename configs rename vars

pjfanning · 2023-12-13T13:51:16Z

I think I've covered most if the review items

pjfanning · 2023-12-14T12:14:39Z

I've just noticed that this throttle breaks some of the microbenchmarks. I think the throttle might be applied to too many frame types.

Running sbt from the incubator-pekko-http dir and executing

http-bench-jmh/Jmh/run -i 3 -wi 3 -f1 -t1 org.apache.pekko.http.impl.engine.http2.H2ServerProcessingBenchmark.*

This will fail with the changes in this PR but runs ok without them. The new throttle is set to fail if there are too many frames. This benchmark does not test rapid resets directly - so it appears that the new throttle is affecting too many frame types.

pjfanning · 2023-12-14T12:45:22Z

I have changed the cost function on the frame throttle to only throttle RST frames. It may be useful to also throttle some other frame types - possibly also:

Continuation Frames
GoAway Frames
WindowUpdate Frames?

* Update H2ClientServerBenchmark.scala * don't throttle header frames * only throttle reset frames * rename params * Update H2ClientServerBenchmark.scala * Update Http2ServerDisableResetSpec.scala

pjfanning · 2023-12-20T00:28:55Z

I changed the code so that the rapid reset check is disabled by default. Would it be possible to have this reviewed again?

In the benchmark, the check does not seem to affect performance.

He-Pin · 2023-12-20T07:12:45Z

I changed the code so that the rapid reset check is disabled by default. Would it be possible to have this reviewed again?

In the benchmark, the check does not seem to affect performance.

How about with a configuration?

pjfanning · 2023-12-20T08:28:17Z

These are the results of running the H2ClientServerBenchmark on my laptop (with the change in this PR, resetFrameThrottleInterval=0s means no throttle applied and is the default). As you may see there is a small impact on my laptop, 0s runs are a little faster than 1s runs.

[info] Benchmark                                       (minStrictEntitySize)  (requestbody)  (resetFrameThrottleInterval)  (responsetype)   Mode  Cnt      Score      Error  Units
[info] H2ClientServerBenchmark.benchRequestProcessing                      1          empty                            1s          strict  thrpt    5  34987.925 ±  911.714  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1          empty                            1s  closedelimited  thrpt    5  29428.729 ± 2196.091  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1          empty                            0s          strict  thrpt    5  36503.237 ± 2168.155  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1          empty                            0s  closedelimited  thrpt    5  31649.809 ± 1056.275  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1    singleframe                            1s          strict  thrpt    5  34068.047 ± 1009.244  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1    singleframe                            1s  closedelimited  thrpt    5  29889.829 ± 2166.454  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1    singleframe                            0s          strict  thrpt    5  35286.571 ± 2411.328  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1    singleframe                            0s  closedelimited  thrpt    5  29821.582 ± 2270.763  ops/s

pjfanning · 2023-12-20T08:31:45Z

without my PR changes (ie latest main branch code) - this seems to show that the PR changes don't appear to affect performance unless you enable the throttle (non-default setting)

[info] Benchmark                                       (minStrictEntitySize)  (requestbody)  (responsetype)   Mode  Cnt      Score      Error  Units
[info] H2ClientServerBenchmark.benchRequestProcessing                      1          empty          strict  thrpt    5  35597.887 ± 2219.899  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1          empty  closedelimited  thrpt    5  30468.592 ± 1722.882  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1    singleframe          strict  thrpt    5  34457.236 ± 2100.593  ops/s
[info] H2ClientServerBenchmark.benchRequestProcessing                      1    singleframe  closedelimited  thrpt    5  29022.231 ± 5096.366  ops/s

mdedetrich

lgtm, would be good to get an approval from someone else.

laglangyue · 2023-12-20T11:22:00Z

...tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2SpecWithMaterializer.scala

+      }
+  }
+
+  protected /* To make ByteFlag warnings go away */ abstract class TestSetupWithoutHandshake {


The comment after protected doesn't seem elegant

this code was moved from another class - I didn't write the original comment

https://github.com/apache/incubator-pekko-http/pull/344/files/a58637af2969f314cd900b44cb87523dc60e61fd#diff-1bf6a966e29f3f599b7126edc971a2eb45087a0f37ff3b578389f38f257424a5L1809

yeah, Should we keep it orgin? If it's not necessary, I still hope to be able to modify it.

pjfanning · 2024-01-19T13:02:32Z

closing due to #394

pjfanning force-pushed the http2-reset-frame branch from 868cb91 to 4903df0 Compare October 29, 2023 14:57

pjfanning force-pushed the http2-reset-frame branch from 39b36b9 to 2556190 Compare October 30, 2023 13:16

pjfanning force-pushed the http2-reset-frame branch 2 times, most recently from e271157 to efd0bd8 Compare November 8, 2023 19:12

pjfanning changed the title ~~[DRAFT] HTTP/2 rapid reset mitigation~~ HTTP/2 rapid reset mitigation Nov 8, 2023

pjfanning requested review from jrudolph, mdedetrich, raboof, He-Pin and nvollmar and removed request for mdedetrich November 8, 2023 21:06

pjfanning force-pushed the http2-reset-frame branch from 94e6db5 to ece1b14 Compare November 12, 2023 08:54

pjfanning requested a review from gmethvin November 13, 2023 12:02

He-Pin reviewed Nov 14, 2023

View reviewed changes

project/MiMa.scala Outdated Show resolved Hide resolved

He-Pin reviewed Nov 14, 2023

View reviewed changes

http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala Outdated Show resolved Hide resolved

He-Pin reviewed Nov 14, 2023

View reviewed changes

http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala Outdated Show resolved Hide resolved

pjfanning force-pushed the http2-reset-frame branch from 2f404ac to 8a529e5 Compare November 14, 2023 12:42

mdedetrich reviewed Dec 8, 2023

View reviewed changes

http-core/src/main/resources/reference.conf Outdated Show resolved Hide resolved

mdedetrich requested changes Dec 8, 2023

View reviewed changes

project/MiMa.scala Outdated Show resolved Hide resolved

pjfanning mentioned this pull request Dec 8, 2023

uptake sbt-pekko-build #365

Merged

pjfanning marked this pull request as draft December 8, 2023 21:27

pjfanning changed the title ~~HTTP/2 rapid reset mitigation~~ [DRAFT] HTTP/2 rapid reset mitigation Dec 8, 2023

pjfanning added 2 commits December 13, 2023 13:38

remove imports

d6ab487

pjfanning force-pushed the http2-reset-frame branch from bbd2833 to d6ab487 Compare December 13, 2023 12:41

Update http2-rapid-reset-configs.backwards.excludes

7d7ffae

pjfanning marked this pull request as ready for review December 13, 2023 13:50

pjfanning changed the title ~~[DRAFT] HTTP/2 rapid reset mitigation~~ HTTP/2 rapid reset mitigation Dec 13, 2023

pjfanning marked this pull request as draft December 14, 2023 12:10

pjfanning changed the title ~~HTTP/2 rapid reset mitigation~~ [DRAFT] HTTP/2 rapid reset mitigation Dec 14, 2023

pjfanning added 2 commits December 14, 2023 13:22

don't throttle header frames

2d7f0a1

only throttle reset frames

e0c6e12

pjfanning added 4 commits December 18, 2023 19:02

rename params

9f55d7c

Update Http2ServerDisableResetSpec.scala

88a2c1e

Rapid reset bench (#2)

8cac7d9

* Update H2ClientServerBenchmark.scala * don't throttle header frames * only throttle reset frames * rename params * Update H2ClientServerBenchmark.scala * Update Http2ServerDisableResetSpec.scala

disable throttle by default

a58637a

pjfanning changed the title ~~[DRAFT] HTTP/2 rapid reset mitigation~~ HTTP/2 rapid reset mitigation Dec 20, 2023

pjfanning marked this pull request as ready for review December 20, 2023 00:27

mdedetrich approved these changes Dec 20, 2023

View reviewed changes

laglangyue reviewed Dec 20, 2023

View reviewed changes

pjfanning modified the milestone: 1.0.0 Dec 28, 2023

rename methods

b3fcafb

pjfanning mentioned this pull request Jan 2, 2024

Http2 frame type throttle #394

Merged

pjfanning marked this pull request as draft January 2, 2024 21:25

pjfanning closed this Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP/2 rapid reset mitigation #344

HTTP/2 rapid reset mitigation #344

pjfanning commented Oct 29, 2023 •

edited

Loading

jrudolph commented Oct 30, 2023

pjfanning commented Nov 13, 2023

He-Pin Nov 14, 2023

pjfanning Nov 14, 2023

He-Pin Nov 15, 2023

pjfanning Nov 15, 2023

mdedetrich left a comment •

edited

Loading

pjfanning commented Dec 13, 2023

pjfanning commented Dec 14, 2023 •

edited

Loading

pjfanning commented Dec 14, 2023

pjfanning commented Dec 20, 2023

He-Pin commented Dec 20, 2023

pjfanning commented Dec 20, 2023 •

edited

Loading

pjfanning commented Dec 20, 2023 •

edited

Loading

mdedetrich left a comment

laglangyue Dec 20, 2023

pjfanning Dec 20, 2023

pjfanning Dec 20, 2023

laglangyue Dec 20, 2023

pjfanning commented Jan 19, 2024

HTTP/2 rapid reset mitigation #344

HTTP/2 rapid reset mitigation #344

Conversation

pjfanning commented Oct 29, 2023 • edited Loading

jrudolph commented Oct 30, 2023

pjfanning commented Nov 13, 2023

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

mdedetrich left a comment • edited Loading

Choose a reason for hiding this comment

pjfanning commented Dec 13, 2023

pjfanning commented Dec 14, 2023 • edited Loading

pjfanning commented Dec 14, 2023

pjfanning commented Dec 20, 2023

He-Pin commented Dec 20, 2023

pjfanning commented Dec 20, 2023 • edited Loading

pjfanning commented Dec 20, 2023 • edited Loading

mdedetrich 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

pjfanning commented Jan 19, 2024

pjfanning commented Oct 29, 2023 •

edited

Loading

mdedetrich left a comment •

edited

Loading

pjfanning commented Dec 14, 2023 •

edited

Loading

pjfanning commented Dec 20, 2023 •

edited

Loading

pjfanning commented Dec 20, 2023 •

edited

Loading