Skip to content

v2.31.0 — what the firewall writes down

Choose a tag to compare

@sean-e-dietrich sean-e-dietrich released this 18 Sep 19:41
1408066

v2.31.0

What the firewall writes down.

Two halves of one surface, in both directions — what a record keeps, and how a log record is sent — and both were found by reviewing 2.30.0's own work rather than by planning. That is now the third release running where that is where the items came from.

PHP 8.1–8.5 supported. One behaviour change on upgrade, below.

What changes on upgrade

  • Block records no longer keep the visitor's cookies, most headers, or the request body. record_request is a new per-backend allowlist. If you were relying on a stored Cookie header to prove two blocks are the same actor, that is the thing this removes and record_request is how you get it back.

Everything else is additive: DeferredHandler does nothing until you wire it, and nested handler blocks in logger: are a shape that previously could not be expressed at all.

One new warning on configurations that change nothing: firewall-doctor counts block records written before this release that still hold cookies or headers, so you know whether there is anything to act on.

Fixed

  • Block-list records kept the visitor's credentials (#375)

    Every block record held the whole cookie jar and header set, verbatim — session cookie, Authorization, challenge pass — and bin/firewall-block --show --json printed them back.

                              before   after
    token in STORED record?    YES       no
    session in STORED record?  YES       no
    

    Three things made that worse than it first reads. The block list is the artifact operators share: pasted into a ticket, read out on a call, and with SharedStorage replicated to every node. A record outlives the request by design, for the ban's duration, which under blocking_escalation is days. And the session cookie is not the firewall's to hold — it belongs to the application in front of it.

    It predates 2.30.0. It is fixed now because #368 made a challenge pass worth something to whoever holds it, and the block list turned out to be where they were written down next to the address that earned them.

    storage:
      config:
        record_request:
          cookies: []                                      # default: none
          headers: [user-agent, referer, accept-language]   # default: a short list
          query: ["*"]                                      # default: everything
          body: []                                          # default: none

    An allowlist rather than a denylist, because a denylist is a promise to have thought of every header name a framework might invent. ["*"] opts a bucket back in.

    The four defaults are deliberately not the same, and follow where the risk actually is. Cookies: none — the session-cookie problem is unambiguous. Headers: the ones that describe a client rather than authenticate it. Body: none, because a blocked login attempt has the password in it. Query: everything, because for a scanner — the commonest reason anyone reads a block record — the query string is the attack, and redacting it by default would gut the record for its main use. That last one is the one to think about for your own site; narrow it if your URLs carry reset tokens or API keys.

    Two things it deliberately does not do. It does not touch records already written — redaction happens on the way in, which is the only place it can, since redacting in --show's output would leave the credential in the store where a shared list replicates it and a backup keeps it. And there is no command that scrubs them in place: a record's remaining ban time cannot be read back portably across the backends, so rewriting one would silently reset its ban to a full term, turning a privacy fix into a change in how long people are blocked for. firewall-doctor counts them instead.

Added

  • Remote log handlers, off the request path (#379)

    Every Monolog handler that speaks HTTP makes a blocking round trip inside write(), once per record, on a component that logs per decision:

    // Monolog\Handler\SlackWebhookHandler::write()
    Curl\Util::execute($ch);

    So a log service having a bad afternoon becomes a slow site — the hazard 2.30.0 refused for metrics, where StatsD is UDP-only and the missing TCP mode is a deliberate refusal. LoggingFactory catches a handler that throws; nothing caught one that hangs, and a slow site is harder to diagnose than a failed one.

    logger:
      - class: Kanopi\Firewall\Logging\Handler\DeferredHandler
        args:
          - class: Monolog\Handler\LogglyHandler
            args: ["%env(LOGGLY_TOKEN)%", Monolog\Level::Warning]
          - 0
    handle()   →  buffer in memory, return immediately
    shutdown   →  fastcgi_finish_request()   ← the visitor is served here
               →  flush the buffer to the wrapped handler
    

    Deliberately not a thirteenth HTTP handler: Monolog's are maintained, and reimplementing the Datadog, Loki and Splunk payload formats is a treadmill. What was missing is a way to get any of them off the request path.

    The documentation now leads with the answer that is better than this one and needs nothing from the package — write JSON lines and let Vector, Fluent Bit or Filebeat ship them, where delivery, retries and backpressure are handled by software built for it. DeferredHandler is for deployments that cannot.

    It also says plainly what deferring does not fix: it cannot bound how long the flush takes, and an FPM worker held after the response is still a worker held. SocketHandler has timeouts; the curl-based handlers expose none.

  • Wrapping handlers is configurable at all (#379)

    The larger half, and it was not in the plan. A wrapping handler takes another handler as its first argument, and logger: args could only carry scalars — so the documented answer for FingersCrossedHandler, BufferHandler and Filter had always been "write PHP instead".

    logger:
      # Keep debug records in memory; write them all only if something goes wrong.
      - class: Monolog\Handler\FingersCrossedHandler
        args:
          - class: Monolog\Handler\StreamHandler
            args: [/var/log/firewall/firewall.log, Monolog\Level::Debug]
          - Monolog\Level::Error

    A nested {class, args} block is built as a handler, with the same is_a(HandlerInterface) check the top level has had since the CWE-470 fix — the nested path is no less attacker-reachable — and depth bounded rather than trusted. A nested array that is not a handler spec is left exactly as it was, so a handler whose constructor genuinely takes an array is unaffected.

Two things worth recording about how this went

A test found a real defect while it was being written. A rejected nested handler spec stayed a raw array and was splatted into a constructor typed HandlerInterface, producing a TypeError — an \Error, which a host catching \Exception never sees, out of the one class whose entire design is to reject a bad block silently rather than fatal on it.

One line carries @codeCoverageIgnore, and it is worth knowing which. Neither fastcgi_finish_request() nor litespeed_finish_request() exists on any SAPI the test suite runs on. Defining a global stub in the bootstrap would have reached it at the cost of making it exist for every other test, which is a worse trade than the gap. Everything around it is covered, including that the visitor is released before the flush.

Upgrading

$ composer require kanopi/firewall:^2.31
$ vendor/bin/firewall-doctor config/firewall.yml

firewall-doctor is the one to run on this upgrade: it reports how many existing block records still hold cookies or headers, and warns if record_request has been set to keep everything.

Full changelog: v2.30.0...v2.31.0