From 898c7e71dae28aa046bdd3e075ecf0d6aa364142 Mon Sep 17 00:00:00 2001 From: Rich Bayliss Date: Fri, 10 Jul 2020 15:02:07 +0100 Subject: [PATCH] bug: Fix unhandled promise rejection When invoking iptables-restore it can fail. This wasn't handled and this makes sure that it fails gracefully. Change-type: patch Signed-off-by: Rich Bayliss --- src/lib/firewall.ts | 93 ++++++++++++++++++++--------------- src/lib/iptables.ts | 21 +++++--- test/13-device-config.spec.ts | 5 +- test/29-firewall.spec.ts | 47 +++++++++++++++--- test/lib/mocked-iptables.ts | 57 ++++++++++++++++++--- 5 files changed, 158 insertions(+), 65 deletions(-) diff --git a/src/lib/firewall.ts b/src/lib/firewall.ts index a467ed9b..5292c39f 100644 --- a/src/lib/firewall.ts +++ b/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}`); + } + } } diff --git a/src/lib/iptables.ts b/src/lib/iptables.ts index 3918a627..af48a838 100644 --- a/src/lib/iptables.ts +++ b/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((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, ), ); } diff --git a/test/13-device-config.spec.ts b/test/13-device-config.spec.ts index 6c3ec03b..f7221243 100644 --- a/test/13-device-config.spec.ts +++ b/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(); }); diff --git a/test/29-firewall.spec.ts b/test/29-firewall.spec.ts index ad7f9adb..fed0ed8f 100644 --- a/test/29-firewall.spec.ts +++ b/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 = {}; + let loggerSpy: sinon.SinonSpy; + let logSpy: sinon.SinonSpy; + let apiEndpoint: string; let listenPort: number; - let dockerStub: sinon.SinonStubbedInstance; 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', () => { diff --git a/test/lib/mocked-iptables.ts b/test/lib/mocked-iptables.ts index 9ade9b91..77b6e9ab 100644 --- a/test/lib/mocked-iptables.ts +++ b/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; @@ -94,8 +103,37 @@ export interface MockedState { export type MockedConext = (state: MockedState) => Promise; 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; };