Send attack reports after the response is flushed - #46
Open
lbajsarowicz wants to merge 2 commits into
Open
Conversation
Report::sendReport() ran a synchronous POST to the report endpoint inside the request, before the block decision, with a 5 second timeout. Every matched request paid the round trip and, when the endpoint was slow or unreachable, up to 5 seconds; under a flood of blocked requests PHP-FPM workers were held for that long each. Build the payload while the request stack is still up, then post it from a shutdown function after fastcgi_finish_request() has flushed the response. The client no longer waits for the report; the payload, timeout and dashboard behaviour are unchanged. sendReport() stays as it was for direct callers.
This file contains hidden or 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
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.
Problem
Plugin\Shield::aroundDispatchcallsReport::sendReport()as soon asWaf::matchRequest()returns anything, before the block decision, and forreport-action rules too.sendReport()is a blocking curl POST toshield.sansec.iowithsetTimeout(5).The client therefore waits for that POST on every matched request:
The worker-pool cost is worse than the latency. A worker is occupied for the whole POST, so with a pool of 50 workers and a 5 s timeout the site can absorb
50 / 5 = 10matched requests per second before every worker is sitting in curl. A trivial flood of requests that hit a WAF rule (exactly the traffic the module exists to catch) saturates the pool, and legitimate traffic queues behind it. The blocked attacker pays nothing; the shop pays.Design
Reportgains two public methods and keepssendReport()untouched for any third-party caller:sendReportDeferred(RequestInterface $request, array $rules)— builds the JSON payload right away, pushes it onto an in-process queue, and registersflushDeferredReportswithregister_shutdown_function()the first time it is called. A$shutdownRegisteredflag keeps the registration to one per process even if several requests match in a single worker lifetime, and the queue means several matches within one request all get reported.flushDeferredReports()— drains the queue, callsfastcgi_finish_request()when that function exists, and posts each payload. It empties the queue before posting, so a second call is a no-op.Plugin\Shieldchanges by one line:sendReportbecomessendReportDeferred. The block decision and the 403 response are untouched.Ordering in
Bootstrap::run()is what makes this work:$application->launch()runs the front controller (and this plugin), and only then$response->sendResponse()echoes the headers and body. Shutdown functions run after the script body, i.e. aftersendResponse(). Under PHP-FPMfastcgi_finish_request()closes the connection to the web server at that point, so the client is served while the worker still holds the POST.Notes on the edge cases:
sendReport()reads$request->getContent()and callsProductMetadataInterface::getVersion(), which inMagento\Framework\App\ProductMetadatagoes throughCacheInterface(mage-version) and, on a miss, throughComposerInformation. At shutdown the cache backend connection (Redis, DB) may already be gone, and the request body stream is not guaranteed readable afterfastcgi_finish_request(). Serialising insendReportDeferred()(while the application stack is still fully up) removes that whole class of failure and guarantees a byte-identical payload. The only observable difference is thattimestampis now the moment of the match rather than the moment of the POST, which is arguably more correct.exitpaths: Magento does notexiton the normal flow, andregister_shutdown_functioncallbacks run onexit()as well, so an early exit somewhere else does not lose the report.fastcgi_finish_request()does not exist there, so the call is skipped and the POST simply happens at shutdown instead of mid-dispatch, no worse than today.What does not change
Cookie,Set-Cookie,Authorization), same serializer. The POST body is byte-identical apart fromtimestamp, which now marks the match.setTimeout(5). Because the client is no longer waiting, that 5 s now bounds only how long a worker can be occupied after the response has been flushed. It no longer bounds the user's page load.blockandreportactions are still sent, and the blocked response is still 403 with theaccess_denied.phtmltemplate.magento/frameworkonly.Rejected alternatives
magento/framework-message-queue: adds a dependency the module does not have, requires operators to run a consumer (and to notice when it dies), delays the dashboard by the consumer's poll interval, and, with the DB queue fallback, parks full request bodies, headers and IPs inqueue_messagerows. That is attack payloads and PII at rest in the merchant's database for as long as the queue is not drained. Wrong trade for a module whose value is that it is small.Reportsimple but scatters lifecycle handling into the plugin and makes the queueing untestable. The plugin stays a one-line call.Testing
Unit tests (
vendor/bin/phpunit Test, 63 tests green):Test/Model/ReportTest.php— new. Covers thatsendReportDeferred()posts nothing at call time; thatflushDeferredReports()then posts one payload per deferred report, to the configured URL, with the expected JSON; that a second flush is a no-op; and that nothing is queued or posted whenreport_enabledis off.flushDeferredReports()is public precisely so the flush can be driven directly.register_shutdown_functioncannot be fired from a test.Test/Plugin/ShieldTest.php— two new cases: a matchedreportrule callssendReportDeferred()once andsendReport()never while dispatch still proceeds; a matchedblockrule defers the report, logs the block, and returns the 403 instead of dispatching.Test/RequestStub.phpgainedgetHeaders(),getFiles()andgetScheme(), whichReportneeds to build a payload.Manual scenario. The response must not wait for a dead endpoint:
Point the module at a listener that accepts the connection and never answers:
Trigger the built-in test rule and measure:
Before this change:
totalis ~5.0 s. The 403 body arrives only after curl inside the worker gives up. After this change:status=403withtotalin the low tens of milliseconds. Thenclistener still shows the connection opening, andtop/fpm statusshows the worker busy for a further 5 s, which is the timeout now bounding worker occupancy rather than page load.Re-point
report_urlat the real endpoint and confirm the attack shows up in the dashboard immediately, and thatvar/log/sansec_shield.logrecords the block as before.