Make the accessory firewall actually restrict anything

Testing the fleet firewall against a real UFW found the rules were
decorative. Two separate faults, both invisible to unit tests.

The generated comment contained an apostrophe — "postgres for db-1's
dependants". ufw rejects the whole rule with `ERROR: Invalid syntax`
however the shell quotes it, and `ufw --force enable` still succeeds, so
the firewall came up hard with the hole never opened. Comments are built
from app, accessory and server names, so this is now sanitised rather
than merely avoided in today's strings.

More seriously, Docker bypasses ufw. Publishing a port writes ACCEPT
rules into the FORWARD path that are consulted before ufw's chain, so
`ufw allow from X to any port 5432` restricted nothing — the rule was
accepted, showed up in `ufw status`, and left Postgres reachable from
every host on the network. Verified: an unrelated VM connected to the
database with the rules in place.

Accessory rules now also write DOCKER-USER entries, the one chain Docker
guarantees to consult first and never rewrite. Allowed sources RETURN,
everything else for that port is DROPped. The DROP is inserted before the
RETURNs because each `-I ... 1` pushes the previous entry down — appending
it instead would land after the RETURN Docker keeps at the end of the
chain, where it is never reached. Rules are deleted before being inserted
so re-running harden cannot stack duplicates, and persisted with
netfilter-persistent since iptables loses them on reboot.

Verified live: outsider blocked, both declared replicas still connected,
chain unchanged and still enforcing after three consecutive runs.

Also adds `--on` to setup, which every other fan-out command already had.

Author: devalade

src/cli/commands/harden.ts

@@ -113,6 +113,17 @@ export async function cmdHarden(cwd: string, options: { config?: string; on?: st
         for (const rule of extra) {
           ui.info(`  allowed ${rule.port}/tcp${rule.from ? ` from ${rule.from}` : ''} — ${rule.comment}`);
         }
+
+        // Accessory ports are Docker's, and Docker's own FORWARD rules are
+        // consulted before ufw's — so the rules above are accepted, appear in
+        // `ufw status`, and restrict nothing. DOCKER-USER is what actually holds.
+        const dockerRules = sec.dockerUserRules(extra);
+        for (const cmd of dockerRules) {
+          await executor.exec(cmd);
+        }
+        if (dockerRules.length > 0) {
+          changes.push('DOCKER-USER: container ports restricted to their declared consumers');
+        }
       }
 
       // List installed pm2 systemd units so we can spot stale ones from a previous

src/cli/commands/setup.ts

@@ -21,6 +21,7 @@ const DEPLOY_USER = 'deploy';
 interface SetupOptions {
   config?: string;
   noDeployUser?: boolean;
+  on?: string;
 }
 
 export async function cmdSetup(cwd: string, options: SetupOptions): Promise<void> {
@@ -45,7 +46,7 @@ export async function cmdSetup(cwd: string, options: SetupOptions): Promise<void
         ui.outro('Server ready — run: shipnode deploy');
       }
     },
-    { configPath: options.config, includeEmpty: true },
+    { configPath: options.config, includeEmpty: true, serverName: options.on },
   );
 }
 

src/cli/index.ts

@@ -64,6 +64,7 @@ program
   .command('setup')
   .description('Setup a new server with required dependencies')
   .option('--no-deploy-user', 'Skip creating the deploy user (advanced)')
+  .option('--on <server>', 'Provision one server instead of every server in the workspace')
   .option('--config <path>', 'Use a specific config file')
   // Commander turns `--no-deploy-user` into `deployUser: false`, not
   // `noDeployUser: true`, so passing opts straight through silently ignored the

src/domain/networking.ts

@@ -87,6 +87,16 @@ export interface FirewallRule {
   port: number;
   from?: string;
   comment: string;
+  /**
+   * Whether the listener is a Docker published port.
+   *
+   * Docker inserts its own ACCEPT rules into the FORWARD path, which are
+   * evaluated before ufw's — so `ufw allow from X to any port 5432` does not
+   * restrict a container's port at all, and the port stays open to everything.
+   * Rules flagged here get an additional DOCKER-USER entry, the one chain Docker
+   * guarantees to consult first.
+   */
+  docker?: boolean;
 }
 
 /** The host-side port of a Docker `-p` mapping, if it publishes one. */
@@ -128,7 +138,7 @@ export function fleetFirewallRules(config: ShipnodeConfig, serverName: string):
     if (app.fleet.port === 80 || app.fleet.port === 443) continue;
     add({
       port: app.fleet.port,
-      comment: `${app.name} replica port (load balancer ingress)`,
+      comment: `shipnode ${app.name} replica port`,
     });
   }
 
@@ -155,7 +165,7 @@ export function fleetFirewallRules(config: ShipnodeConfig, serverName: string):
 
     for (const port of ports) {
       for (const from of consumers) {
-        add({ port, from, comment: `${name} for ${serverName}'s dependants` });
+        add({ port, from, comment: `shipnode ${name} accessory`, docker: true });
       }
     }
   }

src/infrastructure/provisioning/security.ts

@@ -51,8 +51,20 @@ export function ufwConfigureCommands(extra: FirewallRule[] = []): string[] {
   ];
 }
 
+/**
+ * ufw rejects a rule outright — `ERROR: Invalid syntax` — when the comment
+ * contains a quote or an apostrophe, however the shell quotes it. Comments are
+ * built from app, accessory and server names, so this has to be defended rather
+ * than merely avoided in the strings shipnode happens to generate today. A
+ * rejected rule is the worst failure mode available: `ufw --force enable` still
+ * succeeds, so the firewall comes up hard with the hole never opened.
+ */
+export function sanitizeUfwComment(comment: string): string {
+  return comment.replace(/['"\\`$]/g, ' ').replace(/\s+/g, ' ').trim().slice(0, 200);
+}
+
 export function ufwAllowRule(rule: FirewallRule): string {
-  const comment = `comment ${JSON.stringify(rule.comment)}`;
+  const comment = `comment "${sanitizeUfwComment(rule.comment)}"`;
   return rule.from === undefined
     ? `ufw allow ${rule.port}/tcp ${comment}`
     : `ufw allow from ${rule.from} to any port ${rule.port} proto tcp ${comment}`;
@@ -95,3 +107,61 @@ export function fail2banApplyConfigCommand(): string {
 export function fail2banEnableCommand(): string {
   return `${SUDO}; $SUDO systemctl enable fail2ban && $SUDO systemctl restart fail2ban`;
 }
+
+/**
+ * Restrict Docker-published ports, which ufw cannot.
+ *
+ * Docker writes its own ACCEPT rules into the FORWARD path when it publishes a
+ * port. Those are consulted before ufw's chain, so a container port stays open
+ * to the whole network no matter what `ufw allow from ...` says — the rule is
+ * accepted, appears in `ufw status`, and does nothing. An accessory is always a
+ * container, so every accessory rule needs this.
+ *
+ * DOCKER-USER is the one chain Docker promises to evaluate first and never
+ * rewrite. Allowed sources RETURN (falling through to Docker's own ACCEPT);
+ * everything else for that port is dropped.
+ *
+ * Ordering is why the DROP is inserted before the RETURNs: each `-I ... 1`
+ * pushes the previous entry down, so inserting DROP first leaves it last.
+ * Appending it with `-A` would place it after the RETURN that Docker keeps at
+ * the end of the chain, where it would never be reached.
+ */
+export function dockerUserRules(rules: FirewallRule[]): string[] {
+  const docker = rules.filter((rule) => rule.docker);
+  if (docker.length === 0) return [];
+
+  const ports = [...new Set(docker.map((rule) => rule.port))];
+  const commands: string[] = [`${SUDO}`];
+
+  for (const port of ports) {
+    const sources = docker
+      .filter((rule) => rule.port === port && rule.from !== undefined)
+      .map((rule) => rule.from!);
+    if (sources.length === 0) continue;
+
+    // Re-running harden must not stack duplicates, so delete each rule we are
+    // about to add first. `|| true` because a missing rule is the normal case.
+    for (const source of sources) {
+      commands.push(`$SUDO iptables -D DOCKER-USER -p tcp --dport ${port} -s ${source} -j RETURN 2>/dev/null || true`);
+    }
+    commands.push(`$SUDO iptables -D DOCKER-USER -p tcp --dport ${port} -j DROP 2>/dev/null || true`);
+
+    commands.push(`$SUDO iptables -I DOCKER-USER 1 -p tcp --dport ${port} -j DROP`);
+    for (const source of [...sources].reverse()) {
+      commands.push(`$SUDO iptables -I DOCKER-USER 1 -p tcp --dport ${port} -s ${source} -j RETURN`);
+    }
+  }
+
+  if (commands.length === 1) return [];
+
+  // iptables rules are lost on reboot, and a firewall that silently stops
+  // applying is worse than one that was never configured.
+  commands.push(
+    'command -v netfilter-persistent >/dev/null 2>&1 || ' +
+    '{ DEBIAN_FRONTEND=noninteractive $SUDO apt-get install -y -qq iptables-persistent >/dev/null 2>&1 || true; }',
+    '$SUDO netfilter-persistent save >/dev/null 2>&1 || ' +
+    'echo "shipnode: could not persist iptables rules; they will be lost on reboot" >&2',
+  );
+
+  return [commands.join('; ')];
+}

tests/unit/networking.test.ts

@@ -1,7 +1,7 @@
 import { describe, expect, it } from 'vitest';
 import { ShipnodeConfigSchema } from '../../src/config/schema.js';
 import { accessoryHostVar, fleetFirewallRules, isLoopbackOnly } from '../../src/domain/networking.js';
-import { ufwConfigureCommands } from '../../src/infrastructure/provisioning/security.js';
+import { dockerUserRules, sanitizeUfwComment, ufwAllowRule, ufwConfigureCommands } from '../../src/infrastructure/provisioning/security.js';
 import type { ShipnodeConfig } from '../../src/shared/types.js';
 
 function workspace(overrides: Record<string, unknown> = {}): unknown {
@@ -65,8 +65,8 @@ describe('fleetFirewallRules', () => {
     const rules = fleetFirewallRules(parsed(), 'db-1');
 
     expect(rules).toEqual([
-      { port: 5432, from: '10.0.0.11', comment: "postgres for db-1's dependants" },
-      { port: 5432, from: '10.0.0.12', comment: "postgres for db-1's dependants" },
+      { port: 5432, from: '10.0.0.11', comment: 'shipnode postgres accessory', docker: true },
+      { port: 5432, from: '10.0.0.12', comment: 'shipnode postgres accessory', docker: true },
     ]);
   });
 
@@ -104,7 +104,7 @@ describe('fleetFirewallRules', () => {
 
     expect(rules).toContainEqual({
       port: 8080,
-      comment: 'api replica port (load balancer ingress)',
+      comment: 'shipnode api replica port',
     });
   });
 
@@ -213,3 +213,66 @@ describe('cross-server accessory addressing', () => {
     expect(parsed.success).toBe(true);
   });
 });
+
+describe('sanitizeUfwComment', () => {
+  it('strips the characters ufw rejects a rule for', () => {
+    // ufw answers `ERROR: Invalid syntax` and adds nothing, however the shell
+    // quotes it — while `ufw --force enable` still succeeds. The firewall comes
+    // up hard with the hole never opened, which is the worst available outcome.
+    expect(sanitizeUfwComment("postgres for db-1's dependants")).toBe('postgres for db-1 s dependants');
+    expect(sanitizeUfwComment('a "quoted" name')).toBe('a quoted name');
+    expect(sanitizeUfwComment('back\\slash and `tick` and $var')).toBe('back slash and tick and var');
+  });
+
+  it('caps length, since comments come from user-controlled names', () => {
+    expect(sanitizeUfwComment('x'.repeat(500))).toHaveLength(200);
+  });
+
+  it('produces a rule ufw can parse for every generated comment', () => {
+    const rule = ufwAllowRule({ port: 5432, from: '10.0.0.11', comment: "it's \"fine\"" });
+
+    expect(rule).toBe('ufw allow from 10.0.0.11 to any port 5432 proto tcp comment "it s fine"');
+  });
+});
+
+describe('dockerUserRules', () => {
+  const accessory = (from: string) => ({ port: 5432, from, comment: 'pg', docker: true });
+
+  it('drops after the allowed sources, never before', () => {
+    // Each `-I ... 1` pushes the previous entry down, so the DROP must be
+    // inserted first to end up last. Insert it after and it shadows every
+    // RETURN, cutting the accessory off from the replicas that need it.
+    const [script] = dockerUserRules([accessory('10.0.0.11'), accessory('10.0.0.12')]);
+
+    const drop = script.indexOf('-I DOCKER-USER 1 -p tcp --dport 5432 -j DROP');
+    const first = script.indexOf('-I DOCKER-USER 1 -p tcp --dport 5432 -s 10.0.0.11 -j RETURN');
+    expect(drop).toBeGreaterThan(-1);
+    expect(first).toBeGreaterThan(drop);
+  });
+
+  it('never appends, which would land after Docker own trailing RETURN', () => {
+    const [script] = dockerUserRules([accessory('10.0.0.11')]);
+
+    expect(script).not.toContain('-A DOCKER-USER');
+  });
+
+  it('deletes before inserting, so re-running harden does not stack duplicates', () => {
+    const [script] = dockerUserRules([accessory('10.0.0.11')]);
+
+    const del = script.indexOf('-D DOCKER-USER -p tcp --dport 5432 -s 10.0.0.11 -j RETURN');
+    const ins = script.indexOf('-I DOCKER-USER 1 -p tcp --dport 5432 -s 10.0.0.11 -j RETURN');
+    expect(del).toBeGreaterThan(-1);
+    expect(ins).toBeGreaterThan(del);
+  });
+
+  it('persists the rules, which iptables loses on reboot', () => {
+    const [script] = dockerUserRules([accessory('10.0.0.11')]);
+
+    expect(script).toContain('netfilter-persistent save');
+  });
+
+  it('emits nothing for a host-listening port, which ufw already covers', () => {
+    expect(dockerUserRules([{ port: 8080, comment: 'replica port' }])).toEqual([]);
+    expect(dockerUserRules([])).toEqual([]);
+  });
+});