DEV Community

Cover image for A driver that quietly does nothing is worse than one that isn't there
Nasrul Hazim
Nasrul Hazim

Posted on

A driver that quietly does nothing is worse than one that isn't there

TL;DR

  • A method that can't do its job has three options: do it, throw, or return "I can't" as a value. What it must never do is return normally having done nothing.
  • I gave a firewall abstraction two extra methods — enforces() and unavailableReason() — so "not implemented here" became something a caller can act on instead of something it has to infer.
  • The security fix that actually landed wasn't a firewall at all. It was binding published ports to loopback, because a port bound to 127.0.0.1 is closed no matter what any firewall says or fails to say.
  • Fail closed on unrecognised input. An unknown exposure value binds loopback — a typo must never open a port.
  • If two lists in your codebase have to agree, derive both from one predicate and write the test that asserts they partition the set.

Yesterday I wrote about stubs that answer politely — the fakes and empty columns that make a system look complete while it does nothing. Today's work was the next layer of the same problem, and it's the more dangerous one.

A stub that returns an empty array is at least inert. What I spent the day on was code that reported success for work it had never done.

The record of intent that no packet ever meets

There was a network_policies table. Rows were written on every provision, from the first day the provisioning pipeline existed. Ingress rules, egress rules, allowed CIDRs, ports. All of it stored, validated, displayed in the UI.

Nothing ever read it.

Not "read it and got it wrong" — nothing read it at all. There was no code path anywhere that turned a row in that table into a rule on any machine. The deployment pipeline had a step called apply network policies, that step wrote the rows, the step returned success, and the pipeline reported the deployment as fully provisioned with policy applied.

That's a record of intent that no packet ever encounters. And it's a nastier bug than an unimplemented feature, because it produces evidence. An operator looking at that screen has every reason to believe their ingress is restricted.

So the fix was a contract — but the interesting part of the contract isn't apply():

interface FirewallContract
{
    /** @param list<FirewallRuleSpec> $rules */
    public function apply(array $rules): void;

    /** Read back from the enforcement point, not from the database. */
    public function current(): array;

    public function teardown(): void;

    /**
     * Whether this driver can actually filter packets.
     *
     * False means callers must not report policy as applied. It exists so
     * "not implemented here" is a value a step can act on, rather than
     * something inferred from a no-op.
     */
    public function enforces(): bool;

    /** Why enforcement is unavailable, when enforces() is false. */
    public function unavailableReason(): ?string;
}
Enter fullscreen mode Exit fullscreen mode

Those last two methods are the whole point of the change. enforces() makes capability an explicit return value rather than something you deduce by reading the implementation and noticing it has an empty body.

And the only implementation that ships right now is the unenforced one, whose apply() throws. Every provider family resolves to it, each with its own reason naming the actual mechanism it would need — the DOCKER-USER chain for one runtime, the routing mesh for another, the CNI for the cluster case.

The step that used to report success now records Skipped, with the reason attached. The pipeline's test expectation changed from "20 steps succeeded" to "18 succeeded, 2 skipped." That diff — a green number getting smaller — is the actual deliverable.

Two steps stopped claiming work they had not done. That is a system getting more correct while its dashboard gets worse-looking, which is the trade you want and the one that's hardest to sell.

What I deliberately didn't ship

No real iptables driver. Writing DOCKER-USER rules against a host you can't verify against is how somebody locks themselves out of their own machine over SSH, and I'd rather ship a driver that says "I cannot" than one that half-applies.

A firewall that half-applies is worse than one that says it can't. Same principle as the no-op, one step further along.

Fix the exposure where the exposure is

Here's the part that reframed the whole issue for me.

The actual security problem wasn't the missing firewall driver. It's a well-documented Docker behaviour: published ports install DNAT and FORWARD rules that iptables evaluates before ufw's INPUT chain. So a node with a perfectly correct deny-inbound baseline still answers on every published port. ufw status says closed. nmap says otherwise. Both are telling the truth about different chains.

The instinct is to reach for a wider firewall baseline. Wrong instinct — the baseline is correct for what it covers. The fix is that a published port should bind an address:

final readonly class PortMapping
{
    public const LOCAL_BIND = '127.0.0.1';
    public const PUBLIC_BIND = '0.0.0.0';

    public function bindAddress(): string
    {
        return $this->public ? self::PUBLIC_BIND : self::LOCAL_BIND;
    }

    /** The value for `docker run -p`. Never a bare `host:container`. */
    public function publishArg(): string
    {
        return $this->bindAddress().':'.$this->publish.':'.$this->container;
    }
}
Enter fullscreen mode Exit fullscreen mode

A port bound to loopback is invisible from outside the host whatever any firewall does or fails to do. Containers still reach each other by network alias. Anything that must be public goes through the reverse proxy, which is the one deliberate exception and writes 0.0.0.0 literally rather than inheriting it — it's the front door, and port 80 has to keep answering or ACME renewals fail.

Three design decisions in that small class worth stealing:

Fail closed on garbage. An unrecognised exposure value binds loopback. Not an exception, not a default-to-public — loopback. A typo in a config file must not open a port to the internet.

Write the limitation into the code, not the wiki. One orchestrator genuinely can't do this: its routing mesh publishes on every node and offers no bind address. mode=host narrows it to nodes running a task and no further. So a published port there is world-facing and the cluster firewall is the only control — and that sentence lives in a comment next to the code that can't fix it, rather than in a doc someone will find after the incident.

Verify the guard fires. The regression test fails on any -p argument that goes back to a bare host:container. I verified it by putting the old code back and watching it fail, because a regression guard nobody has seen fire is indistinguishable from one that cannot:

it('never publishes a port without a bind address', function () {
    $args = app(ContainerRuntime::class)->runArgumentsFor($workload);

    foreach (portArgs($args) as $arg) {
        expect($arg)->toMatch('/^(127\.0\.0\.1|0\.0\.0\.0):\d+:\d+$/');
    }
});
Enter fullscreen mode Exit fullscreen mode

"I can't" as an exception type

Separate thread, same idea. I added a native runtime — deploying a workload straight onto a VM with systemd, no containers. Air-gapped and hardened estates need it, and containers aren't always on the table.

That runtime has to satisfy the same contract as the container runtimes, and some of that contract it genuinely cannot honour. scale() on a single VM has no honest implementation.

final class UnsupportedByRuntime extends RuntimeException
{
    public static function capability(string $runtime, string $capability, string $reason): self
    {
        return new self("{$runtime} cannot {$capability}: {$reason}");
    }
}
Enter fullscreen mode Exit fullscreen mode

The alternative — returning quietly — would let an operator believe a workload had been scaled. The exception isn't a failure mode, it's the honest answer to a question that has no other answer. A named exception type also makes it greppable: "what can't this runtime do?" becomes a search rather than an archaeology exercise.

Note it's a different tool from enforces(). Use a query method when the caller can reasonably route around the gap (the pipeline can skip a step and record why). Use an exception when there is no sensible alternative path and continuing would be a lie. Getting that split right is most of the design.

Capability is per-provider, not per-type

The bug that made me split the axes properly:

Only one provider type was marked "ready", even though every adopted-VM type — cloud droplet, on-prem box, whatever — resolves to the same real SSH driver, the same agent, the same runtime once it has credentials. So an operator with a cloud VM had to register it as "Bare Metal". A lie the form tells once and a support ticket repeats forever.

Two things came out of fixing it.

One predicate, two lists. The provider form had a hand-written literal list of ready types, and a "coming soon" list computed from isReady(). So the moment a type became ready it vanished from both — excluded from one, filtered out of the other. Another type had been sitting in that hole for a while: a real driver that had never once appeared on the form.

Both lists now derive from the same predicate, and there's a test asserting they partition the enum. If two collections in your code have to agree, the fix is never "remember to update both."

it('partitions every provider type into ready or coming soon', function () {
    $ready = InfraProviderType::readyTypes();
    $soon  = InfraProviderType::comingSoonTypes();

    expect($ready->merge($soon)->sort()->values())
        ->toEqual(collect(InfraProviderType::cases())->sort()->values())
        ->and($ready->intersect($soon))->toBeEmpty();
});
Enter fullscreen mode Exit fullscreen mode

Two axes, not one. Where the machine came from and how workloads run on it are independent. A bare-metal box can host containers; a hardened one cannot. So the container/native choice moved onto the per-provider capability set, not the type enum:

/**
 * Whether this provider deploys workloads as containers.
 *
 * Per provider rather than per type on purpose: a bare-metal box can host
 * containers and a hardened one cannot, so the type is the wrong axis.
 */
public bool $containers = true,
Enter fullscreen mode Exit fullscreen mode

Defaulting to true keeps every provider that predates the key behaving exactly as it did. New capability flags should default to the old behaviour — otherwise adding a flag is a silent migration.

And "ready" got a definition worth writing down: deployable, not complete. A type is ready if a real driver, agent and runtime back it — while its load balancer and some provisioners are still simulated and still declared as such. The UI renders that as a per-capability list rather than a single badge, because collapsing it to one boolean is exactly what overclaims.

One more: the checklist where you tick which components a provider supports grew a warning when the provider has no provisioners at all. Ticking a box there restricts what the engine may schedule; it doesn't install anything. A restriction reading as a promise is the same class of bug as everything else on this page.

The takeaway

Every one of these is the same rule wearing a different hat:

Doing nothing must be distinguishable from doing the thing.

At the method level, that's an exception type. At the contract level, it's a capability query. At the pipeline level, it's a Skipped status carrying a reason. At the UI level, it's a per-capability list instead of a green badge.

None of it is clever. All of it is the difference between a system you can reason about and one that is confidently wrong in a direction you'll discover from a customer.

What's next

The rules table is now the desired state a future enforcer reads, which means the enforcer is a well-defined piece of work rather than an open question — with a contract that already tells it what it must never do. When I do write the real driver, the first test is that it can't be mistaken for the unenforced one.

Top comments (0)