Merge pull request #2135 from balena-os/fix-iptables-input-flush

Replace BALENA-FIREWALL rule in INPUT chain instead of flushing

src/lib/firewall.ts

@@ -170,6 +170,18 @@ export async function applyFirewallMode(mode: string) {
 				  }
 				: [];
 
+		// Get position of BALENA-FIREWALL rule in the INPUT chain for both iptables & ip6tables
+		const v4Position = await iptables.getRulePosition(
+			'INPUT',
+			'BALENA-FIREWALL',
+			4,
+		);
+		const v6Position = await iptables.getRulePosition(
+			'INPUT',
+			'BALENA-FIREWALL',
+			6,
+		);
+
 		// get an adaptor to manipulate iptables rules...
 		const ruleAdaptor = iptables.getDefaultRuleAdaptor();
 
@@ -178,6 +190,37 @@ export async function applyFirewallMode(mode: string) {
 			.build()
 			.forTable('filter', (filter) =>
 				filter
+					.forChain('INPUT', (chain) => {
+						// Delete & insert to v4 tables
+						if (v4Position !== -1) {
+							chain.addRule({
+								action: iptables.RuleAction.Delete,
+								target: 'BALENA-FIREWALL',
+								family: 4,
+							});
+						}
+						chain.addRule({
+							action: iptables.RuleAction.Insert,
+							id: v4Position > 0 ? v4Position : 1,
+							target: 'BALENA-FIREWALL',
+							family: 4,
+						});
+						// Delete & insert to v6 tables
+						if (v6Position !== -1) {
+							chain.addRule({
+								action: iptables.RuleAction.Delete,
+								target: 'BALENA-FIREWALL',
+								family: 6,
+							});
+						}
+						chain.addRule({
+							action: iptables.RuleAction.Insert,
+							id: v6Position > 0 ? v6Position : 1,
+							target: 'BALENA-FIREWALL',
+							family: 6,
+						});
+						return chain;
+					})
 					.forChain(BALENA_FIREWALL_CHAIN, (chain) =>
 						chain
 							.addRule(prepareChain)
@@ -190,16 +233,6 @@ export async function applyFirewallMode(mode: string) {
 								action: iptables.RuleAction.Append,
 								target: 'REJECT',
 							}),
-					)
-					.forChain('INPUT', (chain) =>
-						chain
-							.addRule({
-								action: iptables.RuleAction.Flush,
-							})
-							.addRule({
-								action: iptables.RuleAction.Append,
-								target: 'BALENA-FIREWALL',
-							}),
 					),
 			)
 			.apply(ruleAdaptor);

src/lib/iptables.ts

@@ -3,6 +3,9 @@ import { spawn } from 'child_process';
 import { Readable } from 'stream';
 import { TypedError } from 'typed-error';
 
+import { exec } from './fs-utils';
+import log from './supervisor-console';
+
 export class IPTablesRuleError extends TypedError {
 	public constructor(err: string | Error, public ruleset: string) {
 		super(err);
@@ -13,6 +16,7 @@ export enum RuleAction {
 	Insert = '-I',
 	Append = '-A',
 	Flush = '-F',
+	Delete = '-D',
 }
 export interface Rule {
 	id?: number;
@@ -96,6 +100,14 @@ export function convertToRestoreRulesFormat(rules: Rule[]): string {
 				}
 				if (rule.chain) {
 					args.push(rule.chain);
+					// Optionally push a rule to a specific position in the chain
+					if (
+						(rule.action === RuleAction.Insert ||
+							rule.action === RuleAction.Delete) &&
+						rule.id
+					) {
+						args.push(rule.id?.toString() ?? '1');
+					}
 				}
 				if (rule.proto) {
 					args.push(`-p ${rule.proto}`);
@@ -291,3 +303,34 @@ async function applyRules(rules: Rule | Rule[], adaptor: RuleAdaptor) {
 
 	await adaptor(processedRules);
 }
+
+class StdError extends TypedError {}
+
+export async function getRulePosition(
+	chain: string,
+	ruleMatch: string,
+	family: 4 | 6,
+): Promise<number> {
+	const cmd = `ip${
+		family === 6 ? '6' : ''
+	}tables -L ${chain} -n --line-numbers`;
+	try {
+		const { stdout, stderr } = await exec(cmd);
+		if (stderr) {
+			throw new StdError(stderr);
+		}
+		const line = stdout.split('\n').find((l) => l.includes(ruleMatch));
+		if (!line) {
+			return -1;
+		}
+		return parseInt(line.split(' ')[0], 10);
+	} catch (e: unknown) {
+		log.error(
+			`Received ${
+				e instanceof StdError ? 'stderr' : 'error'
+			} querying iptables ${chain} chain:`,
+			(e as Error).message ?? e ?? 'Unknown error',
+		);
+		return -1;
+	}
+}

test/integration/lib/firewall.spec.ts

@@ -10,6 +10,7 @@ import * as iptablesMock from '~/test-lib/mocked-iptables';
 import * as dbFormat from '~/src/device-state/db-format';
 
 import constants = require('~/lib/constants');
+import * as iptables from '~/lib/iptables';
 import { RuleAction, Rule } from '~/lib/iptables';
 import { log } from '~/lib/supervisor-console';
 
@@ -86,7 +87,7 @@ describe('lib/firewall', function () {
 
 					// expect that we jump to the firewall chain...
 					expectRule({
-						action: RuleAction.Append,
+						action: RuleAction.Insert,
 						target: 'BALENA-FIREWALL',
 						chain: 'INPUT',
 						family: 4,
@@ -122,7 +123,7 @@ describe('lib/firewall', function () {
 
 					// expect that we jump to the firewall chain...
 					expectRule({
-						action: RuleAction.Append,
+						action: RuleAction.Insert,
 						target: 'BALENA-FIREWALL',
 						chain: 'INPUT',
 						family: 4,
@@ -179,7 +180,7 @@ describe('lib/firewall', function () {
 
 					// expect that we jump to the firewall chain...
 					expectRule({
-						action: RuleAction.Append,
+						action: RuleAction.Insert,
 						target: 'BALENA-FIREWALL',
 						chain: 'INPUT',
 						family: 4,
@@ -238,7 +239,7 @@ describe('lib/firewall', function () {
 
 					// expect that we jump to the firewall chain...
 					expectRule({
-						action: RuleAction.Append,
+						action: RuleAction.Insert,
 						target: 'BALENA-FIREWALL',
 						chain: 'INPUT',
 						family: 4,
@@ -269,6 +270,65 @@ describe('lib/firewall', function () {
 				expect(loggerSpy.called).to.be.true;
 			}, iptablesMock.realRuleAdaptor);
 		});
+
+		// TODO: The way that iptables & mocked-iptables are implemented does not
+		// translate to well-written, side-effect free tests. While this test passes,
+		// it's not able to check that BALENA-FIREWALL was inserted at the correct
+		// position in the INPUT rule chain.
+		// We should refactor the test setup for iptables to write rules to a file,
+		// and test whether that file can be applied successfully with `iptables-restore --test`.
+		it('should only replace the BALENA-FIREWALL rule from INPUT chain', async () => {
+			const acceptRuleOnPort = (port: number) => ({
+				action: RuleAction.Append,
+				proto: 'tcp',
+				matches: [`--dport ${port}`],
+				target: 'ACCEPT',
+			});
+
+			await iptablesMock.whilstMocked(
+				async ({ hasAppliedRules, expectRule }) => {
+					// clear the spies...
+					loggerSpy.resetHistory();
+					logSpy.resetHistory();
+
+					// Add some rules to INPUT: 3 unrelated rules,
+					// and BALENA-FIREWALL at position 4.
+					await iptables
+						.build()
+						.forTable('filter', (filter) =>
+							filter.forChain('INPUT', (chain) =>
+								chain
+									.addRule(acceptRuleOnPort(80))
+									.addRule(acceptRuleOnPort(443))
+									.addRule(acceptRuleOnPort(22))
+									.addRule({
+										action: RuleAction.Append,
+										target: 'BALENA-FIREWALL',
+										chain: 'INPUT',
+									}),
+							),
+						)
+						.apply(iptablesMock.fakeRuleAdaptor);
+
+					// set the firewall to be in off mode...
+					await config.set({ firewallMode: 'off' });
+					await hasAppliedRules;
+
+					// Unrelated rules should not have been flushed
+					expectRule(acceptRuleOnPort(80));
+					expectRule(acceptRuleOnPort(443));
+					expectRule(acceptRuleOnPort(22));
+					// TODO: this should be testable but isn't because of the reason above.
+					// expectRule({
+					// 	action: RuleAction.Insert,
+					// 	target: 'BALENA-FIREWALL',
+					// 	chain: 'INPUT',
+					// 	id: 4,
+					// });
+				},
+				iptablesMock.fakeRuleAdaptor,
+			);
+		});
 	});
 
 	describe('Service rules', () => {

test/lib/mocked-iptables.ts

@@ -107,7 +107,7 @@ class FakeRuleAdaptor {
 export const realRuleAdaptor = iptables.getDefaultRuleAdaptor();
 
 const fakeRuleAdaptorManager = new FakeRuleAdaptor();
-const fakeRuleAdaptor = fakeRuleAdaptorManager.getRuleAdaptor();
+export const fakeRuleAdaptor = fakeRuleAdaptorManager.getRuleAdaptor();
 
 // @ts-expect-error Assigning to a RO property
 iptables.getDefaultRuleAdaptor = () => {
@@ -121,11 +121,11 @@ export interface MockedState {
 	clearHistory: () => void;
 }
 
-export type MockedConext = (state: MockedState) => Promise<any>;
+export type MockedContext = (state: MockedState) => Promise<any>;
 
 const applyFirewallRules = firewall.applyFirewallMode;
 export const whilstMocked = async (
-	context: MockedConext,
+	context: MockedContext,
 	ruleAdaptor: iptables.RuleAdaptor = fakeRuleAdaptor,
 ) => {
 	const getOriginalDefaultRuleAdaptor = iptables.getDefaultRuleAdaptor;