From 88cee12ec494a162fe3018bf345ecda672185fdd Mon Sep 17 00:00:00 2001 From: Sander Bruens Date: Wed, 7 Feb 2024 10:35:26 -0500 Subject: [PATCH] feat(server): extend key creation API to allow specifying a port (#1505) * feat(server): extend key creation API to allow specifying a port * Use `validateNumberParam` in `setPortForNewAccessKey`. * Only check if the port is valid if it was specified. * Undo the removal of use of `Promise.all(...)`. * Fix tests. * Only run validation if parameters are provided. * Correct types for API validation methods where `undefined` is a possible return type. --- src/shadowbox/model/access_key.ts | 2 + src/shadowbox/model/errors.ts | 7 +- src/shadowbox/server/api.yml | 8 +-- src/shadowbox/server/manager_service.spec.ts | 54 ++++++++++++++ src/shadowbox/server/manager_service.ts | 52 +++++++------- .../server/server_access_key.spec.ts | 71 +++++++++++++++++++ src/shadowbox/server/server_access_key.ts | 34 +++++++-- 7 files changed, 189 insertions(+), 39 deletions(-) diff --git a/src/shadowbox/model/access_key.ts b/src/shadowbox/model/access_key.ts index d2935fa40..83c30ae92 100644 --- a/src/shadowbox/model/access_key.ts +++ b/src/shadowbox/model/access_key.ts @@ -59,6 +59,8 @@ export interface AccessKeyCreateParams { readonly password?: string; // The data transfer limit to apply to the access key. readonly dataLimit?: DataLimit; + // The port number to use for the access key. + readonly portNumber?: number; } export interface AccessKeyRepository { diff --git a/src/shadowbox/model/errors.ts b/src/shadowbox/model/errors.ts index 4124e2bff..a6445ba10 100644 --- a/src/shadowbox/model/errors.ts +++ b/src/shadowbox/model/errors.ts @@ -24,15 +24,14 @@ class OutlineError extends Error { } export class InvalidPortNumber extends OutlineError { - // Since this is the error when a non-numeric value is passed to `port`, it takes type `string`. - constructor(public port: string) { - super(port); + constructor(public port: number) { + super(`Port ${port} is invalid: must be an integer in range [0, 65353]`); } } export class PortUnavailable extends OutlineError { constructor(public port: number) { - super(port.toString()); + super(`Port ${port} is unavailable`); } } diff --git a/src/shadowbox/server/api.yml b/src/shadowbox/server/api.yml index 48e68b5d8..80eee5526 100644 --- a/src/shadowbox/server/api.yml +++ b/src/shadowbox/server/api.yml @@ -154,13 +154,13 @@ paths: type: string port: type: integer - dataLimit: + limit: $ref: "#/components/schemas/DataLimit" examples: 'No params specified': value: '{"method":"aes-192-gcm"}' 'Provide params': - value: '{"method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","limit":{"bytes":10000}}' + value: '{"method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","port": 12345,"limit":{"bytes":10000}}' responses: '201': description: The newly created access key @@ -196,11 +196,11 @@ paths: type: string port: type: integer - dataLimit: + limit: $ref: "#/components/schemas/DataLimit" examples: '0': - value: '{"id":"123","method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","limit":{"bytes":10000}}' + value: '{"id":"123","method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","port": 12345,"limit":{"bytes":10000}}' responses: '201': description: The newly created access key diff --git a/src/shadowbox/server/manager_service.spec.ts b/src/shadowbox/server/manager_service.spec.ts index 4e6a8dcd3..41933030f 100644 --- a/src/shadowbox/server/manager_service.spec.ts +++ b/src/shadowbox/server/manager_service.spec.ts @@ -531,6 +531,59 @@ describe('ShadowsocksManagerService', () => { done(); }); }); + + it('uses the default port for new keys when no port is provided', async (done) => { + const res = { + send: (httpCode, data) => { + expect(httpCode).toEqual(201); + expect(data.port).toBeDefined(); + responseProcessed = true; // required for afterEach to pass. + }, + }; + await serviceMethod({params: {id: accessKeyId}}, res, done); + }); + + it('uses the provided port when one is provided', async (done) => { + const res = { + send: (httpCode, data) => { + expect(httpCode).toEqual(201); + expect(data.port).toEqual(NEW_PORT); + responseProcessed = true; // required for afterEach to pass. + }, + }; + await serviceMethod({params: {id: accessKeyId, port: NEW_PORT}}, res, done); + }); + + it('rejects ports that are not numbers', async (done) => { + const res = {send: SEND_NOTHING}; + await serviceMethod({params: {id: accessKeyId, port: '1234'}}, res, (error) => { + expect(error.statusCode).toEqual(400); + responseProcessed = true; // required for afterEach to pass. + done(); + }); + }); + + it('rejects invalid port numbers', async (done) => { + const res = {send: SEND_NOTHING}; + await serviceMethod({params: {id: accessKeyId, port: 1.4}}, res, (error) => { + expect(error.statusCode).toEqual(400); + responseProcessed = true; // required for afterEach to pass. + done(); + }); + }); + + it('rejects port numbers already in use', async (done) => { + const server = new net.Server(); + server.listen(NEW_PORT, async () => { + const res = {send: SEND_NOTHING}; + await serviceMethod({params: {id: accessKeyId, port: NEW_PORT}}, res, (error) => { + expect(error.statusCode).toEqual(409); + responseProcessed = true; // required for afterEach to pass. + server.close(); + done(); + }); + }); + }); }); } }); @@ -629,6 +682,7 @@ describe('ShadowsocksManagerService', () => { const server = new net.Server(); server.listen(NEW_PORT, async () => { await service.setPortForNewAccessKeys({params: {port: NEW_PORT}}, res, next); + server.close(); }); }); diff --git a/src/shadowbox/server/manager_service.ts b/src/shadowbox/server/manager_service.ts index 91ae444fe..1299b9b55 100644 --- a/src/shadowbox/server/manager_service.ts +++ b/src/shadowbox/server/manager_service.ts @@ -189,7 +189,7 @@ function validateAccessKeyId(accessKeyId: unknown): string { return accessKeyId; } -function validateDataLimit(limit: unknown): DataLimit { +function validateDataLimit(limit: unknown): DataLimit | undefined { if (typeof limit === 'undefined') { return undefined; } @@ -204,7 +204,7 @@ function validateDataLimit(limit: unknown): DataLimit { return limit as DataLimit; } -function validateStringParam(param: unknown, paramName: string): string { +function validateStringParam(param: unknown, paramName: string): string | undefined { if (typeof param === 'undefined') { return undefined; } @@ -218,6 +218,20 @@ function validateStringParam(param: unknown, paramName: string): string { return param; } +function validateNumberParam(param: unknown, paramName: string): number | undefined { + if (typeof param === 'undefined') { + return undefined; + } + + if (typeof param !== 'number') { + throw new restifyErrors.InvalidArgumentError( + {statusCode: 400}, + `Expected a number for ${paramName}, instead got ${param} of type ${typeof param}` + ); + } + return param; +} + // The ShadowsocksManagerService manages the access keys that can use the server // as a proxy using Shadowsocks. It runs an instance of the Shadowsocks server // for each existing access key, with the port and password assigned for that access key. @@ -342,6 +356,7 @@ export class ShadowsocksManagerService { const name = validateStringParam(req.params.name || '', 'name'); const dataLimit = validateDataLimit(req.params.limit); const password = validateStringParam(req.params.password, 'password'); + const portNumber = validateNumberParam(req.params.port, 'port'); const accessKeyJson = accessKeyToApiJson( await this.accessKeys.createNewAccessKey({ @@ -350,13 +365,16 @@ export class ShadowsocksManagerService { name, dataLimit, password, + portNumber, }) ); return accessKeyJson; } catch (error) { logging.error(error); - if (error instanceof errors.InvalidCipher) { + if (error instanceof errors.InvalidCipher || error instanceof errors.InvalidPortNumber) { throw new restifyErrors.InvalidArgumentError({statusCode: 400}, error.message); + } else if (error instanceof errors.PortUnavailable) { + throw new restifyErrors.ConflictError(error.message); } throw error; } @@ -381,10 +399,7 @@ export class ShadowsocksManagerService { return next(); } catch (error) { logging.error(error); - if ( - error instanceof restifyErrors.InvalidArgumentError || - error instanceof restifyErrors.MissingParameterError - ) { + if (error instanceof restifyErrors.HttpError) { return next(error); } return next(new restifyErrors.InternalServerError()); @@ -409,10 +424,7 @@ export class ShadowsocksManagerService { if (error instanceof errors.AccessKeyConflict) { return next(new restifyErrors.ConflictError(error.message)); } - if ( - error instanceof restifyErrors.InvalidArgumentError || - error instanceof restifyErrors.MissingParameterError - ) { + if (error instanceof restifyErrors.HttpError) { return next(error); } return next(new restifyErrors.InternalServerError()); @@ -427,18 +439,11 @@ export class ShadowsocksManagerService { ): Promise { try { logging.debug(`setPortForNewAccessKeys request ${JSON.stringify(req.params)}`); - const port = req.params.port; - if (!port) { + const port = validateNumberParam(req.params.port, 'port'); + if (port === undefined) { return next( new restifyErrors.MissingParameterError({statusCode: 400}, 'Parameter `port` is missing') ); - } else if (typeof port !== 'number') { - return next( - new restifyErrors.InvalidArgumentError( - {statusCode: 400}, - `Expected a numeric port, instead got ${port} of type ${typeof port}` - ) - ); } await this.accessKeys.setPortForNewAccessKeys(port); this.serverConfig.data().portForNewAccessKeys = port; @@ -451,6 +456,8 @@ export class ShadowsocksManagerService { return next(new restifyErrors.InvalidArgumentError({statusCode: 400}, error.message)); } else if (error instanceof errors.PortUnavailable) { return next(new restifyErrors.ConflictError(error.message)); + } else if (error instanceof restifyErrors.HttpError) { + return next(error); } return next(new restifyErrors.InternalServerError(error)); } @@ -556,10 +563,7 @@ export class ShadowsocksManagerService { return next(); } catch (error) { logging.error(error); - if ( - error instanceof restifyErrors.InvalidArgumentError || - error instanceof restifyErrors.MissingParameterError - ) { + if (error instanceof restifyErrors.HttpError) { return next(error); } return next(new restifyErrors.InternalServerError()); diff --git a/src/shadowbox/server/server_access_key.spec.ts b/src/shadowbox/server/server_access_key.spec.ts index 166ef2d49..8f6e98df4 100644 --- a/src/shadowbox/server/server_access_key.spec.ts +++ b/src/shadowbox/server/server_access_key.spec.ts @@ -99,6 +99,77 @@ describe('ServerAccessKeyRepository', () => { }); }); + it('createNewAccessKey throws on creating keys with invalid port', async (done) => { + const repo = new RepoBuilder().build(); + await expectAsyncThrow( + repo.createNewAccessKey.bind(repo, {portNumber: -123}), + errors.InvalidPortNumber + ); + done(); + }); + + it('createNewAccessKey rejects invalid port numbers', async (done) => { + const repo = new RepoBuilder().build(); + await expectAsyncThrow( + repo.createNewAccessKey.bind(repo, {portNumber: 0}), + errors.InvalidPortNumber + ); + await expectAsyncThrow( + repo.createNewAccessKey.bind(repo, {portNumber: -1}), + errors.InvalidPortNumber + ); + await expectAsyncThrow( + repo.createNewAccessKey.bind(repo, {portNumber: 100.1}), + errors.InvalidPortNumber + ); + await expectAsyncThrow( + repo.createNewAccessKey.bind(repo, {portNumber: 65536}), + errors.InvalidPortNumber + ); + done(); + }); + + it('createNewAccessKey rejects specified ports in use', async (done) => { + const portProvider = new PortProvider(); + const port = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().build(); + const server = new net.Server(); + server.listen(port, async () => { + try { + await repo.createNewAccessKey({portNumber: port}); + fail(`createNewAccessKey should reject already used port ${port}.`); + } catch (error) { + expect(error instanceof errors.PortUnavailable); + } + server.close(); + done(); + }); + }); + + it('createNewAccessKey creates keys with the correct default port', async (done) => { + const portProvider = new PortProvider(); + const defaultPort = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().port(defaultPort).build(); + repo.createNewAccessKey().then((accessKey) => { + expect(accessKey).toBeDefined(); + expect(accessKey.proxyParams.portNumber).toEqual(defaultPort); + done(); + }); + }); + + it('createNewAccessKey creates keys with the port correctly', async (done) => { + const portProvider = new PortProvider(); + const defaultPort = await portProvider.reserveNewPort(); + const newPort = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().port(defaultPort).build(); + repo.createNewAccessKey({portNumber: newPort}).then((accessKey) => { + expect(accessKey).toBeDefined(); + expect(accessKey.proxyParams.portNumber).not.toEqual(defaultPort); + expect(accessKey.proxyParams.portNumber).toEqual(newPort); + done(); + }); + }); + it('Creates access keys under limit', async (done) => { const repo = new RepoBuilder().build(); const accessKey = await repo.createNewAccessKey(); diff --git a/src/shadowbox/server/server_access_key.ts b/src/shadowbox/server/server_access_key.ts index d41be524f..ad85ee337 100644 --- a/src/shadowbox/server/server_access_key.ts +++ b/src/shadowbox/server/server_access_key.ts @@ -106,6 +106,10 @@ function isValidCipher(cipher: string): boolean { return true; } +function isValidPort(port: number): boolean { + return Number.isInteger(port) && port > 0 && port <= 65535; +} + // AccessKeyRepository that keeps its state in a config file and uses ShadowsocksServer // to start and stop per-access-key Shadowsocks instances. Requires external validation // that portForNewAccessKeys is valid. @@ -161,15 +165,19 @@ export class ServerAccessKeyRepository implements AccessKeyRepository { }); } + private async isPortAvailable(port: number): Promise { + return this.isExistingAccessKeyPort(port) || !(await isPortUsed(port)); + } + setHostname(hostname: string): void { this.proxyHostname = hostname; } async setPortForNewAccessKeys(port: number): Promise { - if (!Number.isInteger(port) || port < 1 || port > 65535) { - throw new errors.InvalidPortNumber(port.toString()); + if (!isValidPort(port)) { + throw new errors.InvalidPortNumber(port); } - if (!this.isExistingAccessKeyPort(port) && (await isPortUsed(port))) { + if (!(await this.isPortAvailable(port))) { throw new errors.PortUnavailable(port); } this.portForNewAccessKeys = port; @@ -198,15 +206,27 @@ export class ServerAccessKeyRepository implements AccessKeyRepository { } const metricsId = uuidv4(); const password = params?.password ?? generatePassword(); + const encryptionMethod = params?.encryptionMethod || this.NEW_USER_ENCRYPTION_METHOD; + if (encryptionMethod !== this.NEW_USER_ENCRYPTION_METHOD) { + if (!isValidCipher(encryptionMethod)) { + throw new errors.InvalidCipher(encryptionMethod); + } + } - // Validate encryption method. - if (!isValidCipher(encryptionMethod)) { - throw new errors.InvalidCipher(encryptionMethod); + const portNumber = params?.portNumber ?? this.portForNewAccessKeys; + if (portNumber !== this.portForNewAccessKeys) { + if (!isValidPort(portNumber)) { + throw new errors.InvalidPortNumber(portNumber); + } + if (!(await this.isPortAvailable(portNumber))) { + throw new errors.PortUnavailable(portNumber); + } } + const proxyParams = { hostname: this.proxyHostname, - portNumber: this.portForNewAccessKeys, + portNumber, encryptionMethod, password, };