Complete /v1/device/host-config unit tests, modify PATCH route

Change-type: minor
Signed-off-by: Christina Wang <christina@balena.io>

src/device-state.ts

@@ -41,6 +41,8 @@ import {
 import * as dbFormat from './device-state/db-format';
 import * as apiKeys from './lib/api-keys';
 
+const disallowedHostConfigPatchFields = ['local_ip', 'local_port'];
+
 function validateLocalState(state: any): asserts state is TargetState['local'] {
 	if (state.name != null) {
 		if (!validation.isValidShortText(state.name)) {
@@ -128,14 +130,62 @@ function createDeviceStateRouter() {
 			),
 	);
 
-	router.patch('/v1/device/host-config', (req, res) =>
-		hostConfig
-			.patch(req.body)
-			.then(() => res.status(200).send('OK'))
-			.catch((err) =>
-				res.status(503).send(err?.message ?? err ?? 'Unknown error'),
-			),
-	);
+	router.patch('/v1/device/host-config', async (req, res) => {
+		// Because v1 endpoints are legacy, and this endpoint might already be used
+		// by multiple users, adding too many throws might have unintended side effects.
+		// Thus we're simply logging invalid fields and allowing the request to continue.
+
+		try {
+			if (!req.body.network) {
+				log.warn("Key 'network' must exist in PATCH body");
+				// If network does not exist, skip all field validation checks below
+				throw new Error();
+			}
+
+			const { proxy } = req.body.network;
+
+			// Validate proxy fields, if they exist
+			if (proxy && Object.keys(proxy).length) {
+				const blacklistedFields = Object.keys(proxy).filter((key) =>
+					disallowedHostConfigPatchFields.includes(key),
+				);
+
+				if (blacklistedFields.length > 0) {
+					log.warn(`Invalid proxy field(s): ${blacklistedFields.join(', ')}`);
+				}
+
+				if (
+					proxy.type &&
+					!constants.validRedsocksProxyTypes.includes(proxy.type)
+				) {
+					log.warn(
+						`Invalid redsocks proxy type, must be one of ${constants.validRedsocksProxyTypes.join(
+							', ',
+						)}`,
+					);
+				}
+
+				if (proxy.noProxy && !Array.isArray(proxy.noProxy)) {
+					log.warn('noProxy field must be an array of addresses');
+				}
+			}
+		} catch (e) {
+			/* noop */
+		}
+
+		try {
+			// If hostname is an empty string, return first 7 digits of device uuid
+			if (req.body.network?.hostname === '') {
+				const uuid = await config.get('uuid');
+				req.body.network.hostname = uuid?.slice(0, 7);
+			}
+
+			await hostConfig.patch(req.body);
+			res.status(200).send('OK');
+		} catch (err) {
+			res.status(503).send(err?.message ?? err ?? 'Unknown error');
+		}
+	});
 
 	router.get('/v1/device', async (_req, res) => {
 		try {

src/host-config.ts

@@ -43,7 +43,7 @@ const redsocksConfPath = path.join(proxyBasePath, 'redsocks.conf');
 const noProxyPath = path.join(proxyBasePath, 'no_proxy');
 
 interface ProxyConfig {
-	[key: string]: string | string[];
+	[key: string]: string | string[] | number;
 }
 
 interface HostConfig {
@@ -61,7 +61,8 @@ const memoizedAuthRegex = _.memoize(
 );
 
 const memoizedRegex = _.memoize(
-	(proxyField) => new RegExp(proxyField + '\\s*=\\s*([^;\\s]*)\\s*;'),
+	// Add beginning-of-line RegExp to prevent local_ip and local_port static fields from being memoized
+	(proxyField) => new RegExp('^\\s*' + proxyField + '\\s*=\\s*([^;\\s]*)\\s*;'),
 );
 
 async function readProxy(): Promise<ProxyConfig | undefined> {
@@ -93,13 +94,24 @@ async function readProxy(): Promise<ProxyConfig | undefined> {
 	}
 
 	try {
-		const noProxy = await fs.readFile(noProxyPath, 'utf-8');
-		conf.noProxy = noProxy.split('\n');
+		const noProxy = await fs
+			.readFile(noProxyPath, 'utf-8')
+			// Prevent empty newline from being reported as a noProxy address
+			.then((addrs) => addrs.split('\n').filter((addr) => addr !== ''));
+
+		if (noProxy.length) {
+			conf.noProxy = noProxy;
+		}
 	} catch (e) {
 		if (!ENOENT(e)) {
 			throw e;
 		}
 	}
+
+	// Convert port to number per API doc spec
+	if (conf.port) {
+		conf.port = parseInt(conf.port as string, 10);
+	}
 	return conf;
 }
 
@@ -134,9 +146,21 @@ async function setProxy(maybeConf: ProxyConfig | null): Promise<void> {
 		if (_.isArray(conf.noProxy)) {
 			await writeFileAtomic(noProxyPath, conf.noProxy.join('\n'));
 		}
-		const redsocksConf = `${redsocksHeader}${generateRedsocksConfEntries(
-			conf,
-		)}${redsocksFooter}`;
+
+		let currentConf: ProxyConfig | undefined;
+		try {
+			currentConf = await readProxy();
+		} catch (err) {
+			// Noop - current redsocks.conf does not exist
+		}
+
+		// If currentConf is undefined, the currentConf spread will be skipped.
+		// See: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-1.html#conditional-spreads-create-optional-properties
+		const redsocksConf = `
+			${redsocksHeader}\n
+			${generateRedsocksConfEntries({ ...currentConf, ...conf })}
+			${redsocksFooter}
+		`;
 		await writeFileAtomic(redsocksConfPath, redsocksConf);
 	}
 

src/lib/constants.ts

@@ -69,6 +69,7 @@ const constants = {
 	// (this number is used as an upper bound when generating
 	// a random jitter)
 	maxApiJitterDelay: 60 * 1000,
+	validRedsocksProxyTypes: ['socks4', 'socks5', 'http-connect', 'http-relay'],
 };
 
 if (process.env.DOCKER_HOST == null) {

test/41-device-api-v1.spec.ts

@@ -10,6 +10,8 @@ import {
 	SinonFakeTimers,
 } from 'sinon';
 import * as supertest from 'supertest';
+import * as path from 'path';
+import { promises as fs } from 'fs';
 
 import * as appMock from './lib/application-state-mock';
 import * as mockedDockerode from './lib/mocked-dockerode';
@@ -26,8 +28,11 @@ import * as updateLock from '../src/lib/update-lock';
 import * as TargetState from '../src/device-state/target-state';
 import * as targetStateCache from '../src/device-state/target-state-cache';
 import blink = require('../src/lib/blink');
+import constants = require('../src/lib/constants');
 
 import { UpdatesLockedError } from '../src/lib/errors';
+import { SchemaTypeKey } from '../src/config/schema-type';
+import log from '../src/lib/supervisor-console';
 
 describe('SupervisorAPI [V1 Endpoints]', () => {
 	let api: SupervisorAPI;
@@ -725,5 +730,419 @@ describe('SupervisorAPI [V1 Endpoints]', () => {
 		});
 	});
 
+	describe('/v1/device/host-config', () => {
+		// Wrap GET and PATCH /v1/device/host-config tests in the same block to share
+		// common scoped variables, namely file paths and file content
+		const hostnamePath: string = path.join(
+			process.env.ROOT_MOUNTPOINT!,
+			'/etc/hostname',
+		);
+		const proxyBasePath: string = path.join(
+			process.env.ROOT_MOUNTPOINT!,
+			process.env.BOOT_MOUNTPOINT!,
+			'system-proxy',
+		);
+		const redsocksPath: string = path.join(proxyBasePath, 'redsocks.conf');
+		const noProxyPath: string = path.join(proxyBasePath, 'no_proxy');
+
+		/**
+		 * Copies contents of hostname, redsocks.conf, and no_proxy test files with `.template`
+		 * endings to test files without `.template` endings to ensure the same data always
+		 * exists for /v1/device/host-config test suites
+		 */
+		const restoreConfFileTemplates = async (): Promise<void[]> => {
+			return Promise.all([
+				fs.writeFile(
+					hostnamePath,
+					await fs.readFile(`${hostnamePath}.template`),
+				),
+				fs.writeFile(
+					redsocksPath,
+					await fs.readFile(`${redsocksPath}.template`),
+				),
+				fs.writeFile(noProxyPath, await fs.readFile(`${noProxyPath}.template`)),
+			]);
+		};
+
+		// Set hostname & proxy file content to expected defaults
+		before(async () => await restoreConfFileTemplates());
+		afterEach(async () => await restoreConfFileTemplates());
+
+		// Store GET responses for endpoint in variables so we can be less verbose in tests
+		const hostnameOnlyRes =
+			sampleResponses.V1.GET['/device/host-config [Hostname only]'];
+		const hostnameProxyRes =
+			sampleResponses.V1.GET['/device/host-config [Hostname and proxy]'];
+
+		describe('GET /v1/device/host-config', () => {
+			it('returns current host config (hostname and proxy)', async () => {
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(hostnameProxyRes.statusCode)
+					.then((response) => {
+						expect(response.body).to.deep.equal(hostnameProxyRes.body);
+					});
+			});
+
+			it('returns current host config (hostname only)', async () => {
+				await Promise.all([fs.unlink(redsocksPath), fs.unlink(noProxyPath)]);
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(hostnameOnlyRes.statusCode)
+					.then((response) => {
+						expect(response.body).to.deep.equal(hostnameOnlyRes.body);
+					});
+			});
+
+			it('errors if no hostname file exists', async () => {
+				await fs.unlink(hostnamePath);
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(503);
+			});
+		});
+
+		describe('PATCH /v1/device/host-config', () => {
+			let configSetStub: SinonStub;
+			let logWarnStub: SinonStub;
+			let restartServiceSpy: SinonSpy;
+
+			const validProxyReqs: { [key: string]: number[] | string[] } = {
+				ip: ['proxy.example.org', 'proxy.foo.org'],
+				port: [5128, 1080],
+				type: constants.validRedsocksProxyTypes,
+				login: ['user', 'user2'],
+				password: ['foo', 'bar'],
+			};
+
+			// Mock to short-circuit config.set, allowing writing hostname directly to test file
+			const configSetFakeFn = async <T extends SchemaTypeKey>(
+				keyValues: config.ConfigMap<T>,
+			): Promise<void> =>
+				await fs.writeFile(hostnamePath, (keyValues as any).hostname);
+
+			const validatePatchResponse = (res: supertest.Response): void => {
+				expect(res.text).to.equal(
+					sampleResponses.V1.PATCH['/host/device-config'].text,
+				);
+				expect(res.body).to.deep.equal(
+					sampleResponses.V1.PATCH['/host/device-config'].body,
+				);
+			};
+
+			before(() => {
+				configSetStub = stub(config, 'set').callsFake(configSetFakeFn);
+				logWarnStub = stub(log, 'warn');
+			});
+
+			after(() => {
+				configSetStub.restore();
+				logWarnStub.restore();
+			});
+
+			beforeEach(() => {
+				restartServiceSpy = spy(dbus, 'restartService');
+			});
+
+			afterEach(() => {
+				restartServiceSpy.restore();
+			});
+
+			it('updates the hostname with provided string if string is not empty', async () => {
+				await Promise.all([fs.unlink(redsocksPath), fs.unlink(noProxyPath)]);
+
+				const patchBody = { network: { hostname: 'newdevice' } };
+
+				await request
+					.patch('/v1/device/host-config')
+					.send(patchBody)
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(sampleResponses.V1.PATCH['/host/device-config'].statusCode)
+					.then((response) => {
+						validatePatchResponse(response);
+					});
+
+				// Should restart hostname service on successful change
+				expect(restartServiceSpy.callCount).to.equal(1);
+				expect(restartServiceSpy).to.have.been.calledWith('resin-hostname');
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.then((response) => {
+						expect(response.body).to.deep.equal(patchBody);
+					});
+			});
+
+			it('updates hostname to first 7 digits of device uuid when sent invalid hostname', async () => {
+				await Promise.all([fs.unlink(redsocksPath), fs.unlink(noProxyPath)]);
+
+				await request
+					.patch('/v1/device/host-config')
+					.send({ network: { hostname: '' } })
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(sampleResponses.V1.PATCH['/host/device-config'].statusCode)
+					.then((response) => {
+						validatePatchResponse(response);
+					});
+
+				// Should restart hostname service on successful change
+				expect(restartServiceSpy.callCount).to.equal(1);
+				expect(restartServiceSpy).to.have.been.calledWith('resin-hostname');
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.then(async (response) => {
+						const uuidHostname = await config
+							.get('uuid')
+							.then((uuid) => uuid?.slice(0, 7));
+
+						expect(response.body).to.deep.equal({
+							network: { hostname: uuidHostname },
+						});
+					});
+			});
+
+			it('removes proxy when sent empty proxy object', async () => {
+				await request
+					.patch('/v1/device/host-config')
+					.send({ network: { proxy: {} } })
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(sampleResponses.V1.PATCH['/host/device-config'].statusCode)
+					.then(async (response) => {
+						validatePatchResponse(response);
+
+						expect(fs.stat(redsocksPath)).to.be.rejected;
+						expect(fs.stat(noProxyPath)).to.be.rejected;
+					});
+
+				expect(restartServiceSpy.callCount).to.equal(2);
+				expect(restartServiceSpy.args).to.deep.equal([
+					['resin-proxy-config'],
+					['redsocks'],
+				]);
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(hostnameOnlyRes.statusCode)
+					.then((response) => {
+						expect(response.body).to.deep.equal(hostnameOnlyRes.body);
+					});
+			});
+
+			it('updates proxy type when provided valid values', async () => {
+				// Test each proxy patch sequentially to prevent conflicts when writing to fs
+				let restartCallCount = 0;
+				for (const key of Object.keys(validProxyReqs)) {
+					const patchBodyValuesforKey: string[] | number[] =
+						validProxyReqs[key];
+					for (const value of patchBodyValuesforKey) {
+						await request
+							.patch('/v1/device/host-config')
+							.send({ network: { proxy: { [key]: value } } })
+							.set('Accept', 'application/json')
+							.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+							.expect(
+								sampleResponses.V1.PATCH['/host/device-config'].statusCode,
+							)
+							.then((response) => {
+								validatePatchResponse(response);
+							});
+
+						expect(restartServiceSpy.callCount).to.equal(
+							++restartCallCount * 2,
+						);
+
+						await request
+							.get('/v1/device/host-config')
+							.set('Accept', 'application/json')
+							.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+							.expect(hostnameProxyRes.statusCode)
+							.then((response) => {
+								expect(response.body).to.deep.equal({
+									network: {
+										hostname: hostnameProxyRes.body.network.hostname,
+										// All other proxy configs should be unchanged except for any values sent in patch
+										proxy: {
+											...hostnameProxyRes.body.network.proxy,
+											[key]: value,
+										},
+									},
+								});
+							});
+					} // end for (const value of patchBodyValuesforKey)
+					await restoreConfFileTemplates();
+				} // end for (const key in validProxyReqs)
+			});
+
+			it('warns on the supervisor console when provided disallowed proxy fields', async () => {
+				const invalidProxyReqs: { [key: string]: string | number } = {
+					// At this time, don't support changing local_ip or local_port
+					local_ip: '0.0.0.0',
+					local_port: 12345,
+					type: 'invalidType',
+					noProxy: 'not a list of addresses',
+				};
+
+				for (const key of Object.keys(invalidProxyReqs)) {
+					await request
+						.patch('/v1/device/host-config')
+						.send({ network: { proxy: { [key]: invalidProxyReqs[key] } } })
+						.set('Accept', 'application/json')
+						.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+						.expect(200)
+						.then(() => {
+							if (key === 'type') {
+								expect(logWarnStub).to.have.been.calledWith(
+									`Invalid redsocks proxy type, must be one of ${validProxyReqs.type.join(
+										', ',
+									)}`,
+								);
+							} else if (key === 'noProxy') {
+								expect(logWarnStub).to.have.been.calledWith(
+									'noProxy field must be an array of addresses',
+								);
+							} else {
+								expect(logWarnStub).to.have.been.calledWith(
+									`Invalid proxy field(s): ${key}`,
+								);
+							}
+						});
+				}
+			});
+
+			it('replaces no_proxy file with noProxy array from PATCH body', async () => {
+				await request
+					.patch('/v1/device/host-config')
+					.send({ network: { proxy: { noProxy: ['1.2.3.4/5'] } } })
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(sampleResponses.V1.PATCH['/host/device-config'].statusCode)
+					.then((response) => {
+						validatePatchResponse(response);
+					});
+
+				expect(restartServiceSpy.callCount).to.equal(2);
+				expect(restartServiceSpy.args).to.deep.equal([
+					['resin-proxy-config'],
+					['redsocks'],
+				]);
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(hostnameProxyRes.statusCode)
+					.then((response) => {
+						expect(response.body).to.deep.equal({
+							network: {
+								hostname: hostnameProxyRes.body.network.hostname,
+								// New noProxy should be only value in no_proxy file
+								proxy: {
+									...hostnameProxyRes.body.network.proxy,
+									noProxy: ['1.2.3.4/5'],
+								},
+							},
+						});
+					});
+			});
+
+			it('removes no_proxy file when sent an empty array', async () => {
+				await request
+					.patch('/v1/device/host-config')
+					.send({ network: { proxy: { noProxy: [] } } })
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(sampleResponses.V1.PATCH['/host/device-config'].statusCode)
+					.then((response) => {
+						validatePatchResponse(response);
+					});
+
+				expect(restartServiceSpy.callCount).to.equal(2);
+				expect(restartServiceSpy.args).to.deep.equal([
+					['resin-proxy-config'],
+					['redsocks'],
+				]);
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(hostnameProxyRes.statusCode)
+					.then((response) => {
+						expect(response.body).to.deep.equal({
+							network: {
+								hostname: hostnameProxyRes.body.network.hostname,
+								// Reference all properties in proxy object EXCEPT noProxy
+								proxy: {
+									ip: hostnameProxyRes.body.network.proxy.ip,
+									login: hostnameProxyRes.body.network.proxy.login,
+									password: hostnameProxyRes.body.network.proxy.password,
+									port: hostnameProxyRes.body.network.proxy.port,
+									type: hostnameProxyRes.body.network.proxy.type,
+								},
+							},
+						});
+					});
+			});
+
+			it('does not update hostname or proxy when hostname or proxy are undefined', async () => {
+				await request
+					.patch('/v1/device/host-config')
+					.send({ network: {} })
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(sampleResponses.V1.PATCH['/host/device-config'].statusCode)
+					.then((response) => {
+						validatePatchResponse(response);
+					});
+
+				// As no host configs were patched, no services should be restarted
+				expect(restartServiceSpy.callCount).to.equal(0);
+
+				await request
+					.get('/v1/device/host-config')
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(hostnameProxyRes.statusCode)
+					.then((response) => {
+						expect(response.body).to.deep.equal(hostnameProxyRes.body);
+					});
+			});
+
+			it('warns on console when sent a malformed patch body', async () => {
+				await request
+					.patch('/v1/device/host-config')
+					.send({})
+					.set('Accept', 'application/json')
+					.set('Authorization', `Bearer ${apiKeys.cloudApiKey}`)
+					.expect(200)
+					.then(() => {
+						expect(logWarnStub).to.have.been.calledWith(
+							"Key 'network' must exist in PATCH body",
+						);
+					});
+
+				expect(restartServiceSpy.callCount).to.equal(0);
+			});
+		});
+	});
+
 	// TODO: add tests for V1 endpoints
 });

test/data/device-api-responses.json

@@ -42,6 +42,26 @@
         "body": {
           "containerId": "abc123"
         }
+      },
+      "/device/host-config [Hostname only]": {
+        "statusCode": 200,
+        "body": { "network": { "hostname": "foobardevice" } }
+      },
+      "/device/host-config [Hostname and proxy]": {
+        "statusCode": 200,
+        "body": {
+          "network": {
+            "hostname": "foobardevice",
+            "proxy": {
+              "ip": "example.org",
+              "noProxy": ["152.10.30.4", "253.1.1.0/16"],
+              "port": 1080,
+              "type": "socks5",
+              "login": "foo",
+              "password": "bar"
+            }
+          }
+        }
       }
     },
     "POST": {
@@ -74,6 +94,13 @@
         "statusCode": 200,
         "body": {}
       }
+    },
+    "PATCH": {
+      "/host/device-config": {
+        "statusCode": 200,
+        "body": {},
+        "text": "OK"
+      }
     }
   },
   "V2": {

test/data/etc/hostname.template

@@ -0,0 +1 @@
+foobardevice

test/data/mnt/boot/system-proxy/no_proxy

@@ -0,0 +1,2 @@
+152.10.30.4
+253.1.1.0/16

test/data/mnt/boot/system-proxy/no_proxy.template

@@ -0,0 +1,2 @@
+152.10.30.4
+253.1.1.0/16

test/data/mnt/boot/system-proxy/redsocks.conf

@@ -0,0 +1,17 @@
+base {
+    log_debug = off;
+    log_info = on;
+    log = stderr;
+    daemon = off;
+    redirector = iptables;
+}
+
+redsocks {
+    local_ip = 127.0.0.1;
+    local_port = 12345;
+    ip = example.org;
+    port = 1080;
+    type = socks5;
+    login = "foo";
+    password = "bar";
+}

test/data/mnt/boot/system-proxy/redsocks.conf.template

@@ -0,0 +1,17 @@
+base {
+    log_debug = off;
+    log_info = on;
+    log = stderr;
+    daemon = off;
+    redirector = iptables;
+}
+
+redsocks {
+    local_ip = 127.0.0.1;
+    local_port = 12345;
+    ip = example.org;
+    port = 1080;
+    type = socks5;
+    login = "foo";
+    password = "bar";
+}

test/lib/mocked-dbus.ts

@@ -26,6 +26,9 @@ stub(dbus, 'getBus').returns({
 			StartUnit: (_unitName: string) => {
 				// noop
 			},
+			RestartUnit: (_unitName: string, _mode: string) => {
+				// noop
+			},
 		} as any);
 	},
 } as any);