Merge pull request #1392 from balena-io/fix-firewall-promise-rejection

bug: Fix unhandled promise rejection

src/lib/firewall.ts

@@ -4,6 +4,7 @@ import * as config from '../config/index';
 import * as constants from './constants';
 import * as iptables from './iptables';
 import { log } from './supervisor-console';
+import { logSystemMessage } from '../logger';
 
 import * as dbFormat from '../device-state/db-format';
 
@@ -12,7 +13,7 @@ export const initialised = (async () => {
 	await applyFirewall();
 
 	// apply firewall whenever relevant config changes occur...
-	config.on('change', async ({ firewallMode, localMode }) => {
+	config.on('change', ({ firewallMode, localMode }) => {
 		if (firewallMode || localMode != null) {
 			applyFirewall({ firewallMode, localMode });
 		}
@@ -145,48 +146,58 @@ export async function applyFirewallMode(mode: string) {
 
 	log.info(`${LOG_PREFIX} Applying firewall mode: ${mode}`);
 
-	// get an adaptor to manipulate iptables rules...
-	const ruleAdaptor = iptables.getDefaultRuleAdaptor();
+	try {
+		// are we running services in host-network mode?
+		const isServicesInHostNetworkMode = await runningHostBoundServices();
 
-	// are we running services in host-network mode?
-	const isServicesInHostNetworkMode = await runningHostBoundServices();
+		// should we allow only traffic to the balena host services?
+		const returnIfOff: iptables.Rule | iptables.Rule[] =
+			mode === 'off' || (mode === 'auto' && !isServicesInHostNetworkMode)
+				? {
+						comment: `Firewall disabled (${mode})`,
+						action: iptables.RuleAction.Insert,
+						target: 'RETURN',
+				  }
+				: [];
 
-	// should we allow only traffic to the balena host services?
-	const returnIfOff: iptables.Rule | iptables.Rule[] =
-		mode === 'off' || (mode === 'auto' && !isServicesInHostNetworkMode)
-			? {
-					comment: `Firewall disabled (${mode})`,
-					action: iptables.RuleAction.Insert,
-					target: 'RETURN',
-			  }
-			: [];
+		// get an adaptor to manipulate iptables rules...
+		const ruleAdaptor = iptables.getDefaultRuleAdaptor();
 
-	// configure the BALENA-FIREWALL chain...
-	await iptables
-		.build()
-		.forTable('filter', (filter) =>
-			filter
-				.forChain(BALENA_FIREWALL_CHAIN, (chain) =>
-					chain
-						.addRule(prepareChain)
-						.addRule(supervisorAccessRules)
-						.addRule(standardServices)
-						.addRule(standardPolicy)
-						.addRule(returnIfOff),
-				)
-				.forChain('INPUT', (chain) =>
-					chain
-						.addRule({
-							action: iptables.RuleAction.Flush,
-						})
-						.addRule({
-							action: iptables.RuleAction.Append,
-							target: 'BALENA-FIREWALL',
-						}),
-				),
-		)
-		.apply(ruleAdaptor);
+		// configure the BALENA-FIREWALL chain...
+		await iptables
+			.build()
+			.forTable('filter', (filter) =>
+				filter
+					.forChain(BALENA_FIREWALL_CHAIN, (chain) =>
+						chain
+							.addRule(prepareChain)
+							.addRule(supervisorAccessRules)
+							.addRule(standardServices)
+							.addRule(standardPolicy)
+							.addRule(returnIfOff),
+					)
+					.forChain('INPUT', (chain) =>
+						chain
+							.addRule({
+								action: iptables.RuleAction.Flush,
+							})
+							.addRule({
+								action: iptables.RuleAction.Append,
+								target: 'BALENA-FIREWALL',
+							}),
+					),
+			)
+			.apply(ruleAdaptor);
 
-	// all done!
-	log.success(`${LOG_PREFIX} Firewall mode applied`);
+		// all done!
+		log.success(`${LOG_PREFIX} Firewall mode applied`);
+	} catch (err) {
+		logSystemMessage(`${LOG_PREFIX} Firewall mode not applied due to error`);
+		log.error(`${LOG_PREFIX} Firewall mode not applied`);
+		log.error('Error applying firewall mode', err);
+
+		if (err instanceof iptables.IPTablesRuleError) {
+			log.debug(`Ruleset:\r\n${err.ruleset}`);
+		}
+	}
 }

src/lib/iptables.ts

@@ -1,8 +1,13 @@
 import * as _ from 'lodash';
 import { child_process } from 'mz';
 import { Readable } from 'stream';
+import TypedError = require('typed-error');
 
-export class IPTablesRuleError extends Error {}
+export class IPTablesRuleError extends TypedError {
+	public constructor(err: string | Error, public ruleset: string) {
+		super(err);
+	}
+}
 
 export enum RuleAction {
 	Insert = '-I',
@@ -98,10 +103,11 @@ export function convertToRestoreRulesFormat(rules: Rule[]): string {
 				if (rule.matches) {
 					rule.matches.forEach((match) => args.push(match));
 				}
-				if (rule.comment) {
-					args.push('-m comment');
-					args.push(`--comment "${rule.comment}"`);
-				}
+				// TODO: Enable this once the support for it can be confirmed...
+				// if (rule.comment) {
+				// 	args.push('-m comment');
+				// 	args.push(`--comment "${rule.comment}"`);
+				// }
 				if (rule.target) {
 					args.push(`-j ${rule.target}`);
 				}
@@ -151,14 +157,14 @@ const iptablesRestoreAdaptor: RuleAdaptor = async (
 			return;
 		}
 
-		const input = rulesFiles[family];
+		const ruleset = rulesFiles[family];
 		const cmd = family === 'v6' ? 'ip6tables-restore' : 'iptables-restore';
 		await new Promise<string>((resolve, reject) => {
 			const args = ['--noflush', '--verbose'];
 
 			// prepare to pipe the rules into iptables-restore...
 			const stdinStream = new Readable();
-			stdinStream.push(input);
+			stdinStream.push(ruleset);
 			stdinStream.push(null);
 
 			// run the restore...
@@ -185,6 +191,7 @@ const iptablesRestoreAdaptor: RuleAdaptor = async (
 					return reject(
 						new IPTablesRuleError(
 							`Error running iptables: ${stderr.join()} (${args.join(' ')})`,
+							ruleset,
 						),
 					);
 				}

test/13-device-config.spec.ts

@@ -1,6 +1,6 @@
 import { stripIndent } from 'common-tags';
 import { child_process, fs } from 'mz';
-import { SinonStub, stub, spy } from 'sinon';
+import { SinonStub, stub, spy, SinonSpy } from 'sinon';
 
 import { expect } from './lib/chai-config';
 import * as deviceConfig from '../src/device-config';
@@ -14,9 +14,10 @@ const extlinuxBackend = new ExtlinuxConfigBackend();
 const rpiConfigBackend = new RPiConfigBackend();
 
 describe('Device Backend Config', () => {
-	const logSpy = spy(logger, 'logSystemMessage');
+	let logSpy: SinonSpy;
 
 	before(async () => {
+		logSpy = spy(logger, 'logSystemMessage');
 		await prepare();
 	});
 

test/29-firewall.spec.ts

@@ -1,4 +1,4 @@
-import { spy } from 'sinon';
+import _ = require('lodash');
 import { expect } from 'chai';
 
 import * as Docker from 'dockerode';
@@ -7,22 +7,33 @@ import * as sinon from 'sinon';
 
 import * as config from '../src/config';
 import * as firewall from '../src/lib/firewall';
+import * as logger from '../src/logger';
 import * as iptablesMock from './lib/mocked-iptables';
 import * as targetStateCache from '../src/device-state/target-state-cache';
 
 import constants = require('../src/lib/constants');
 import { RuleAction } from '../src/lib/iptables';
+import { log } from '../src/lib/supervisor-console';
 
 describe('Host Firewall', function () {
+	const dockerStubs: Dictionary<sinon.SinonStub> = {};
+	let loggerSpy: sinon.SinonSpy;
+	let logSpy: sinon.SinonSpy;
+
 	let apiEndpoint: string;
 	let listenPort: number;
-	let dockerStub: sinon.SinonStubbedInstance<typeof docker>;
 
 	before(async () => {
-		dockerStub = sinon.stub(docker);
-		dockerStub.listContainers.resolves([]);
-		dockerStub.listImages.resolves([]);
-		dockerStub.getImage.returns({
+		// spy the logs...
+		loggerSpy = sinon.spy(logger, 'logSystemMessage');
+		logSpy = sinon.spy(log, 'error');
+
+		// stub the docker calls...
+		dockerStubs.listContainers = sinon
+			.stub(docker, 'listContainers')
+			.resolves([]);
+		dockerStubs.listImages = sinon.stub(docker, 'listImages').resolves([]);
+		dockerStubs.getImage = sinon.stub(docker, 'getImage').returns({
 			id: 'abcde',
 			inspect: async () => {
 				return {};
@@ -37,13 +48,17 @@ describe('Host Firewall', function () {
 	});
 
 	after(async () => {
-		sinon.restore();
+		for (const stub of _.values(dockerStubs)) {
+			stub.restore();
+		}
+		loggerSpy.restore();
+		logSpy.restore();
 	});
 
 	describe('Basic On/Off operation', () => {
 		it('should confirm the `changed` event is handled', async function () {
 			await iptablesMock.whilstMocked(async ({ hasAppliedRules }) => {
-				const changedSpy = spy();
+				const changedSpy = sinon.spy();
 				config.on('change', changedSpy);
 
 				// set the firewall to be in off mode...
@@ -245,6 +260,22 @@ describe('Host Firewall', function () {
 				},
 			);
 		});
+
+		it('should catch errors when rule changes fail', async () => {
+			await iptablesMock.whilstMocked(async ({ hasAppliedRules }) => {
+				// clear the spies...
+				loggerSpy.resetHistory();
+				logSpy.resetHistory();
+
+				// set the firewall to be in off mode...
+				await config.set({ firewallMode: 'off' });
+				await hasAppliedRules;
+
+				// should have caught the error and logged it
+				expect(logSpy.calledWith('Error applying firewall mode')).to.be.true;
+				expect(loggerSpy.called).to.be.true;
+			}, iptablesMock.realRuleAdaptor);
+		});
 	});
 
 	describe('Supervisor API access', () => {

test/lib/mocked-iptables.ts

@@ -1,9 +1,12 @@
 import _ = require('lodash');
 import { expect } from 'chai';
+import { stub } from 'sinon';
+import { child_process } from 'mz';
 
 import * as firewall from '../../src/lib/firewall';
 import * as iptables from '../../src/lib/iptables';
 import { EventEmitter } from 'events';
+import { Writable } from 'stream';
 
 class FakeRuleAdaptor {
 	private rules: iptables.Rule[];
@@ -80,9 +83,15 @@ class FakeRuleAdaptor {
 	}
 }
 
-const fakeRuleAdaptor = new FakeRuleAdaptor();
+export const realRuleAdaptor = iptables.getDefaultRuleAdaptor();
+
+const fakeRuleAdaptorManager = new FakeRuleAdaptor();
+const fakeRuleAdaptor = fakeRuleAdaptorManager.getRuleAdaptor();
+
 // @ts-expect-error Assigning to a RO property
-iptables.getDefaultRuleAdaptor = () => fakeRuleAdaptor.getRuleAdaptor();
+iptables.getDefaultRuleAdaptor = () => {
+	return fakeRuleAdaptor;
+};
 
 export interface MockedState {
 	hasAppliedRules: Promise<void>;
@@ -94,8 +103,37 @@ export interface MockedState {
 export type MockedConext = (state: MockedState) => Promise<any>;
 
 const applyFirewallRules = firewall.applyFirewallMode;
-export const whilstMocked = async (context: MockedConext) => {
-	fakeRuleAdaptor.clearHistory();
+export const whilstMocked = async (
+	context: MockedConext,
+	ruleAdaptor: iptables.RuleAdaptor = fakeRuleAdaptor,
+) => {
+	const getOriginalDefaultRuleAdaptor = iptables.getDefaultRuleAdaptor;
+
+	const spawnStub = stub(child_process, 'spawn').callsFake(() => {
+		const fakeProc = new EventEmitter();
+		(fakeProc as any).stdout = new EventEmitter();
+
+		const stdin = new Writable();
+		stdin._write = (
+			chunk: Buffer,
+			_encoding: string,
+			callback: (err?: Error) => void,
+		) => {
+			console.log(chunk.toString('utf8'));
+			callback();
+			fakeProc.emit('close', 1);
+		};
+		(fakeProc as any).stdin = stdin;
+
+		return fakeProc as any;
+	});
+
+	// @ts-expect-error Assigning to a RO property
+	iptables.getDefaultRuleAdaptor = () => {
+		return ruleAdaptor;
+	};
+
+	fakeRuleAdaptorManager.clearHistory();
 
 	const applied = new EventEmitter();
 
@@ -106,9 +144,9 @@ export const whilstMocked = async (context: MockedConext) => {
 	};
 
 	await context({
-		expectRule: (rule) => fakeRuleAdaptor.expectRule(rule),
-		expectNoRule: (rule) => fakeRuleAdaptor.expectNoRule(rule),
-		clearHistory: () => fakeRuleAdaptor.clearHistory(),
+		expectRule: (rule) => fakeRuleAdaptorManager.expectRule(rule),
+		expectNoRule: (rule) => fakeRuleAdaptorManager.expectNoRule(rule),
+		clearHistory: () => fakeRuleAdaptorManager.clearHistory(),
 		hasAppliedRules: new Promise((resolve) => {
 			applied.once('applied', () => resolve());
 		}),
@@ -116,4 +154,9 @@ export const whilstMocked = async (context: MockedConext) => {
 
 	// @ts-expect-error Assigning to a RO property
 	firewall.applyFirewallMode = applyFirewallRules;
+
+	spawnStub.restore();
+
+	// @ts-expect-error Assigning to a RO property
+	iptables.getDefaultRuleAdaptor = getOriginalDefaultRuleAdaptor;
 };