diff --git a/src/lib/firewall.ts b/src/lib/firewall.ts index 3aba7d2a..1cb02374 100644 --- a/src/lib/firewall.ts +++ b/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); diff --git a/src/lib/iptables.ts b/src/lib/iptables.ts index 38d1a24c..5e6b5ff5 100644 --- a/src/lib/iptables.ts +++ b/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 { + 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; + } +} diff --git a/test/integration/lib/firewall.spec.ts b/test/integration/lib/firewall.spec.ts index 304ead29..e25fe375 100644 --- a/test/integration/lib/firewall.spec.ts +++ b/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', () => { diff --git a/test/lib/mocked-iptables.ts b/test/lib/mocked-iptables.ts index 2f290813..b864fdd0 100644 --- a/test/lib/mocked-iptables.ts +++ b/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; +export type MockedContext = (state: MockedState) => Promise; const applyFirewallRules = firewall.applyFirewallMode; export const whilstMocked = async ( - context: MockedConext, + context: MockedContext, ruleAdaptor: iptables.RuleAdaptor = fakeRuleAdaptor, ) => { const getOriginalDefaultRuleAdaptor = iptables.getDefaultRuleAdaptor;