diff --git a/src/shadowbox/infrastructure/get_port.spec.ts b/src/shadowbox/infrastructure/get_port.spec.ts index be29cf963..fb18275ce 100644 --- a/src/shadowbox/infrastructure/get_port.spec.ts +++ b/src/shadowbox/infrastructure/get_port.spec.ts @@ -29,15 +29,6 @@ describe('PortProvider', () => { }); }); - describe('freePort', () => { - it('makes port available', () => { - const ports = new get_port.PortProvider(); - ports.addReservedPort(8080); - ports.freePort(8080); - ports.addReservedPort(8080); - }); - }); - describe('reserveFirstFreePort', () => { it('returns free port', async () => { const ports = new get_port.PortProvider(); @@ -54,6 +45,33 @@ describe('PortProvider', () => { expect(await ports.reserveFirstFreePort(9090)).toBeGreaterThan(9091); }); }); + + describe('reserveNewPort', () => { + it('Returns a port not in use', async (done) => { + for (let i = 0; i < 1000; ++i) { + const port = await new get_port.PortProvider().reserveNewPort(); + expect(await get_port.isPortUsed(port)).toBeFalsy(); + } + done(); + }); + }); +}); + +describe('isPortUsed', () => { + it('Identifies a port in use', async (done) => { + const port = 12345; + const server = new net.Server(); + server.listen(port, async () => { + expect(await get_port.isPortUsed(port)).toBeTruthy(); + server.close(); + done(); + }); + }); + it('Identifies a port not in use', async (done) => { + const port = await new get_port.PortProvider().reserveNewPort(); + expect(await get_port.isPortUsed(port)).toBeFalsy(); + done(); + }); }); function listen(): Promise { diff --git a/src/shadowbox/infrastructure/get_port.ts b/src/shadowbox/infrastructure/get_port.ts index a1f976422..1d6c5c1e4 100644 --- a/src/shadowbox/infrastructure/get_port.ts +++ b/src/shadowbox/infrastructure/get_port.ts @@ -53,10 +53,6 @@ export class PortProvider { return port; } } - - freePort(port: number) { - this.reservedPorts.delete(port); - } } function getRandomPortOver1023() { @@ -67,7 +63,7 @@ interface ServerError extends Error { code: string; } -function isPortUsed(port: number): Promise { +export function isPortUsed(port: number): Promise { return new Promise((resolve, reject) => { let isUsed = false; const server = new net.Server(); diff --git a/src/shadowbox/integration_test/test.sh b/src/shadowbox/integration_test/test.sh index 188b52e9b..1e3325308 100755 --- a/src/shadowbox/integration_test/test.sh +++ b/src/shadowbox/integration_test/test.sh @@ -72,7 +72,7 @@ function fail() { function cleanup() { status=$? - (($DEBUG != 0)) || docker-compose down + (($DEBUG != 0)) || docker-compose --project-name=integrationtest down return $status } @@ -89,7 +89,8 @@ function cleanup() { # Sets everything up export SB_API_PREFIX=TestApiPrefix - docker-compose up --build -d + SB_API_URL=https://shadowbox/${SB_API_PREFIX} + docker-compose --project-name=integrationtest up --build -d # Wait for target to come up. wait_for_resource localhost:10080 @@ -111,13 +112,13 @@ function cleanup() { # Create new shadowbox user. # TODO(bemasc): Verify that the server is using the right certificate - declare -r NEW_USER_JSON=$(client_curl --insecure -X POST https://shadowbox/${SB_API_PREFIX}/access-keys) + declare -r NEW_USER_JSON=$(client_curl --insecure -X POST ${SB_API_URL}/access-keys) [[ ${NEW_USER_JSON} == '{"id":"0"'* ]] || fail "Fail to create user" declare -r SS_USER_ARGUMENTS=$(ss_arguments_for_user $NEW_USER_JSON) # Verify that we handle deletions well - client_curl --insecure -X POST https://shadowbox/${SB_API_PREFIX}/access-keys > /dev/null - client_curl --insecure -X DELETE https://shadowbox/${SB_API_PREFIX}/access-keys/1 > /dev/null + client_curl --insecure -X POST ${SB_API_URL}/access-keys > /dev/null + client_curl --insecure -X DELETE ${SB_API_URL}/access-keys/1 > /dev/null # Start Shadowsocks client and wait for it to be ready declare -r LOCAL_SOCKS_PORT=5555 @@ -138,11 +139,22 @@ function cleanup() { || fail "Could not fetch $INTERNET_TARGET_URL through shadowbox." # Verify we can't access the URL anymore after the key is deleted - client_curl --insecure -X DELETE https://shadowbox/${SB_API_PREFIX}/access-keys/0 > /dev/null + client_curl --insecure -X DELETE ${SB_API_URL}/access-keys/0 > /dev/null # Exit code 56 is "Connection reset by peer". client_curl -x socks5h://localhost:$LOCAL_SOCKS_PORT --connect-timeout 1 $INTERNET_TARGET_URL &> /dev/null \ && fail "Deleted access key is still active" || (($? == 56)) + # Verify that we can change the port for new access keys + client_curl --insecure -X PUT -H "Content-Type: application/json" -d '{"port": 12345}' ${SB_API_URL}/server/port-for-new-access-keys \ + || fail "Couldn't change the port for new access keys" + + ACCESS_KEY_JSON=$(client_curl --insecure -X POST ${SB_API_URL}/access-keys \ + || fail "Couldn't get a new access key after changing port") + + if [[ "${ACCESS_KEY_JSON}" != *'"port":12345'* ]]; then + fail "Port for new access keys wasn't changed. Newly created access key: ${ACCESS_KEY_JSON}" + fi + # Verify no errors occurred. readonly SHADOWBOX_LOG=$OUTPUT_DIR/shadowbox-log.txt if docker logs $SHADOWBOX_CONTAINER 2>&1 | tee $SHADOWBOX_LOG | egrep -q "^E|level=error|ERROR:"; then diff --git a/src/shadowbox/model/access_key.ts b/src/shadowbox/model/access_key.ts index 4b48fe06d..21dfdcae0 100644 --- a/src/shadowbox/model/access_key.ts +++ b/src/shadowbox/model/access_key.ts @@ -66,6 +66,8 @@ export interface AccessKeyRepository { removeAccessKey(id: AccessKeyId): boolean; // Lists all existing access keys listAccessKeys(): AccessKey[]; + // Changes the port for new access keys. + setPortForNewAccessKeys(port: number): Promise; // Apply the specified update to the specified access key. // Returns true if successful. renameAccessKey(id: AccessKeyId, name: string): boolean; diff --git a/src/shadowbox/model/errors.ts b/src/shadowbox/model/errors.ts new file mode 100644 index 000000000..90aafb4ea --- /dev/null +++ b/src/shadowbox/model/errors.ts @@ -0,0 +1,34 @@ +// Copyright 2019 The Outline Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +class ShadowboxError extends Error { + constructor(message: string) { + super(message); + // https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html#support-for-newtarget + Object.setPrototypeOf(this, new.target.prototype); + } +} + +export class InvalidPortNumber extends ShadowboxError { + // Since this is the error when a non-numeric value is passed to `port`, it takes type `string`. + constructor(public port: string) { + super(port); + } +} + +export class PortUnavailable extends ShadowboxError { + constructor(public port: number) { + super(port.toString()); + } +} diff --git a/src/shadowbox/server/api.yml b/src/shadowbox/server/api.yml index e162166b3..907d27292 100644 --- a/src/shadowbox/server/api.yml +++ b/src/shadowbox/server/api.yml @@ -28,6 +28,31 @@ paths: '0': value: >- {"name":"My Server","serverId":"000-000-000-000","metricsEnabled":true,"createdTimestampMs":1536613192052,"portForNewAccessKeys":1234} + /server/port-for-new-access-keys: + put: + description: Changes the default port for newly created access keys. This can be a port already used for access keys. + tags: + - Access Key + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + port: + type: number + examples: + '0': + value: '{"port": 12345}' + responses: + '204': + description: The default port was sucessfully changed. + '400': + description: The requested port wasn't an integer from 1 through 65535, or the request had no port parameter. + '409': + description: The requested port was already in use by another service. + /name: put: description: Renames the server diff --git a/src/shadowbox/server/main.ts b/src/shadowbox/server/main.ts index 180ae3a4d..8316d6f58 100644 --- a/src/shadowbox/server/main.ts +++ b/src/shadowbox/server/main.ts @@ -49,43 +49,19 @@ async function exportPrometheusMetrics(registry: prometheus.Registry, port): Pro }); } -function reserveAccessKeyPorts( +function reserveExistingAccessKeyPorts( keyConfig: json_config.JsonConfig, portProvider: PortProvider) { const accessKeys = keyConfig.data().accessKeys || []; const dedupedPorts = new Set(accessKeys.map(ak => ak.port)); dedupedPorts.forEach(p => portProvider.addReservedPort(p)); } -function getPortForNewAccessKeys( - serverConfig: json_config.JsonConfig, - keyConfig: json_config.JsonConfig): number { - if (!serverConfig.data().portForNewAccessKeys) { - // NOTE(2019-01-04): For backward compatibility. Delete after servers have been migrated. - if (keyConfig.data().defaultPort) { - // Migrate setting from keyConfig to serverConfig. - serverConfig.data().portForNewAccessKeys = keyConfig.data().defaultPort; - serverConfig.write(); - delete keyConfig.data().defaultPort; - keyConfig.write(); - } - } - return serverConfig.data().portForNewAccessKeys; -} - -async function reservePortForNewAccessKeys( - portProvider: PortProvider, - serverConfig: json_config.JsonConfig): Promise { - serverConfig.data().portForNewAccessKeys = await portProvider.reserveNewPort(); - serverConfig.write(); - return serverConfig.data().portForNewAccessKeys; -} - async function main() { const verbose = process.env.LOG_LEVEL === 'debug'; const portProvider = new PortProvider(); const accessKeyConfig = json_config.loadFileConfig( getPersistentFilename('shadowbox_config.json')); - reserveAccessKeyPorts(accessKeyConfig, portProvider); + reserveExistingAccessKeyPorts(accessKeyConfig, portProvider); prometheus.collectDefaultMetrics({register: prometheus.register}); @@ -108,12 +84,12 @@ async function main() { logging.debug(`==============`); const DEFAULT_PORT = 8081; - const portNumber = Number(process.env.SB_API_PORT || DEFAULT_PORT); - if (isNaN(portNumber)) { + const apiPortNumber = Number(process.env.SB_API_PORT || DEFAULT_PORT); + if (isNaN(apiPortNumber)) { logging.error(`Invalid SB_API_PORT: ${process.env.SB_API_PORT}`); process.exit(1); } - portProvider.addReservedPort(portNumber); + portProvider.addReservedPort(apiPortNumber); const serverConfig = server_config.readServerConfig(getPersistentFilename('shadowbox_server_config.json')); @@ -160,12 +136,13 @@ async function main() { getPersistentFilename('prometheus/config.yml'), prometheusConfigJson); const prometheusClient = new PrometheusClient(`http://${prometheusLocation}`); + if (!serverConfig.data().portForNewAccessKeys) { + serverConfig.data().portForNewAccessKeys = await portProvider.reserveNewPort(); + serverConfig.write(); + } const accessKeyRepository = new ServerAccessKeyRepository( - portProvider, proxyHostname, accessKeyConfig, shadowsocksServer, prometheusClient); - - const portForNewAccessKeys = getPortForNewAccessKeys(serverConfig, accessKeyConfig) || - await reservePortForNewAccessKeys(portProvider, serverConfig); - accessKeyRepository.enableSinglePort(portForNewAccessKeys); + serverConfig.data().portForNewAccessKeys, proxyHostname, accessKeyConfig, shadowsocksServer, + prometheusClient); const metricsReader = new PrometheusUsageMetrics(prometheusClient); const toMetricsId = (id: AccessKeyId) => { @@ -196,7 +173,7 @@ async function main() { apiServer.use(restify.bodyParser()); bindService(apiServer, apiPrefix, managerService); - apiServer.listen(portNumber, () => { + apiServer.listen(apiPortNumber, () => { logging.info(`Manager listening at ${apiServer.url}${apiPrefix}`); }); diff --git a/src/shadowbox/server/manager_service.spec.ts b/src/shadowbox/server/manager_service.spec.ts index 52752abaa..66b516c6a 100644 --- a/src/shadowbox/server/manager_service.spec.ts +++ b/src/shadowbox/server/manager_service.spec.ts @@ -12,12 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -import {ManualClock} from '../infrastructure/clock'; -import {PortProvider} from '../infrastructure/get_port'; +import * as net from 'net'; + import {InMemoryConfig} from '../infrastructure/json_config'; import {AccessKey, AccessKeyQuota, AccessKeyRepository} from '../model/access_key'; -import {ManagerMetrics} from './manager_metrics'; import {ShadowsocksManagerService} from './manager_service'; import {FakePrometheusClient, FakeShadowsocksServer} from './mocks/mocks'; import {AccessKeyConfigJson, ServerAccessKeyRepository} from './server_access_key'; @@ -28,6 +27,9 @@ interface ServerInfo { name: string; } +const newPort = 12345; +const oldPort = 54321; + describe('ShadowsocksManagerService', () => { // After processing the response callback, we should set // responseProcessed=true. This is so we can detect that first the response @@ -147,6 +149,145 @@ describe('ShadowsocksManagerService', () => { }); }); }); + describe('setPortForNewAccessKeys', () => { + it('changes ports for new access keys', async (done) => { + const repo = getAccessKeyRepository(); + const serverConfig = new InMemoryConfig({} as ServerConfigJson); + const service = new ShadowsocksManagerService('name', serverConfig, repo, null, null); + + const oldKey = await repo.createNewAccessKey(); + const res = { + send: (httpCode) => { + expect(httpCode).toEqual(204); + } + }; + await service.setPortForNewAccessKeys({params: {port: newPort}}, res, () => {}); + const newKey = await repo.createNewAccessKey(); + expect(newKey.proxyParams.portNumber).toEqual(newPort); + expect(oldKey.proxyParams.portNumber).not.toEqual(newPort); + responseProcessed = true; + done(); + }); + + it('changes the server config', async (done) => { + const repo = getAccessKeyRepository(); + const serverConfig = new InMemoryConfig({} as ServerConfigJson); + const service = new ShadowsocksManagerService('name', serverConfig, repo, null, null); + + const res = { + send: (httpCode) => { + expect(httpCode).toEqual(204); + expect(serverConfig.data().portForNewAccessKeys).toEqual(newPort); + responseProcessed = true; + } + }; + await service.setPortForNewAccessKeys({params: {port: newPort}}, res, done); + }); + + it('rejects invalid port numbers', async (done) => { + const repo = getAccessKeyRepository(); + const serverConfig = new InMemoryConfig({} as ServerConfigJson); + const service = new ShadowsocksManagerService('name', serverConfig, repo, null, null); + + const res = { + send: (httpCode) => { + fail( + `setPortForNewAccessKeys should have failed with 400 Bad Request, instead succeeded with code ${ + httpCode}`); + } + }; + const next = (error) => { + // Bad Request + expect(error.statusCode).toEqual(400); + }; + + await service.setPortForNewAccessKeys({params: {port: -1}}, res, next); + await service.setPortForNewAccessKeys({params: {port: 0}}, res, next); + await service.setPortForNewAccessKeys({params: {port: 100.1}}, res, next); + await service.setPortForNewAccessKeys({params: {port: 65536}}, res, next); + + responseProcessed = true; + done(); + }); + + it('rejects port numbers already in use', async (done) => { + const repo = getAccessKeyRepository(); + const serverConfig = new InMemoryConfig({} as ServerConfigJson); + const service = new ShadowsocksManagerService('name', serverConfig, repo, null, null); + + const res = { + send: (httpCode) => { + fail( + `setPortForNewAccessKeys should have failed with 409 Conflict, instead succeeded with code ${ + httpCode}`); + } + }; + const next = (error) => { + // Conflict + expect(error.statusCode).toEqual(409); + responseProcessed = true; + done(); + }; + + const server = new net.Server(); + server.listen(newPort, async () => { + await service.setPortForNewAccessKeys({params: {port: newPort}}, res, next); + }); + }); + + it('accepts port numbers already in use by access keys', async (done) => { + const repo = getAccessKeyRepository(); + const serverConfig = new InMemoryConfig({} as ServerConfigJson); + const service = new ShadowsocksManagerService('name', serverConfig, repo, null, null); + + await service.createNewAccessKey({params: {}}, {send: () => {}}, () => {}); + + await service.setPortForNewAccessKeys({params: {port: newPort}}, {send: () => {}}, () => {}); + + const res = { + send: (httpCode) => { + expect(httpCode).toEqual(204); + responseProcessed = true; + } + }; + + const firstKeyConnection = new net.Server(); + firstKeyConnection.listen(oldPort, async () => { + await service.setPortForNewAccessKeys({params: {port: oldPort}}, res, () => {}); + firstKeyConnection.close(); + done(); + }); + }); + + it('rejects malformed requests', async (done) => { + const repo = getAccessKeyRepository(); + const serverConfig = new InMemoryConfig({} as ServerConfigJson); + const service = new ShadowsocksManagerService('name', serverConfig, repo, null, null); + + const noPort = {params: {}}; + + const res = { + send: (httpCode) => { + fail( + `setPortForNewAccessKeys should have failed with 400 BadRequest, instead succeeded with code ${ + httpCode}`); + } + }; + const next = (error) => { + expect(error.statusCode).toEqual(400); + }; + + await service.setPortForNewAccessKeys(noPort, res, next); + + const nonNumericPort = {params: {port: 'abc'}}; + await service.setPortForNewAccessKeys( + // tslint:disable-next-line: no-any + (nonNumericPort as any) as {params: {port: number}}, res, next); + + responseProcessed = true; + done(); + }); + }); describe('removeAccessKey', () => { it('removes keys', (done) => { @@ -425,7 +566,6 @@ function fakeSharedMetricsReporter(): SharedMetricsPublisher { function getAccessKeyRepository(): AccessKeyRepository { return new ServerAccessKeyRepository( - new PortProvider(), 'hostname', - new InMemoryConfig({accessKeys: [], nextId: 0}), + oldPort, 'hostname', new InMemoryConfig({accessKeys: [], nextId: 0}), new FakeShadowsocksServer(), new FakePrometheusClient({})); } diff --git a/src/shadowbox/server/manager_service.ts b/src/shadowbox/server/manager_service.ts index 8e1642461..09f1d8afa 100644 --- a/src/shadowbox/server/manager_service.ts +++ b/src/shadowbox/server/manager_service.ts @@ -18,6 +18,7 @@ import {makeConfig, SIP002_URI} from 'ShadowsocksConfig/shadowsocks_config'; import {JsonConfig} from '../infrastructure/json_config'; import * as logging from '../infrastructure/logging'; import {AccessKey, AccessKeyQuota, AccessKeyRepository} from '../model/access_key'; +import * as errors from '../model/errors'; import {ManagerMetrics} from './manager_metrics'; import {ServerConfigJson} from './server_config'; @@ -52,6 +53,7 @@ interface RequestParams { name?: string; metricsEnabled?: boolean; quota?: AccessKeyQuota; + port?: number; } interface RequestType { params: RequestParams; @@ -69,9 +71,13 @@ export function bindService( apiServer: restify.Server, apiPrefix: string, service: ShadowsocksManagerService) { apiServer.put(`${apiPrefix}/name`, service.renameServer.bind(service)); apiServer.get(`${apiPrefix}/server`, service.getServer.bind(service)); + apiServer.put( + `${apiPrefix}/server/port-for-new-access-keys`, + service.setPortForNewAccessKeys.bind(service)); apiServer.post(`${apiPrefix}/access-keys`, service.createNewAccessKey.bind(service)); apiServer.get(`${apiPrefix}/access-keys`, service.listAccessKeys.bind(service)); + apiServer.del(`${apiPrefix}/access-keys/:id`, service.removeAccessKey.bind(service)); apiServer.put(`${apiPrefix}/access-keys/:id/name`, service.renameAccessKey.bind(service)); apiServer.put(`${apiPrefix}/access-keys/:id/quota`, service.setAccessKeyQuota.bind(service)); @@ -146,6 +152,39 @@ export class ShadowsocksManagerService { } } + // Sets the default ports for new access keys + public async setPortForNewAccessKeys(req: RequestType, res: ResponseType, next: restify.Next): + Promise { + try { + logging.debug(`setPort[ForNewAccessKeys request ${JSON.stringify(req.params)}`); + if (!req.params.port) { + return next( + new restify.MissingParameterError({statusCode: 400}, 'Parameter `port` is missing')); + } + + const port = req.params.port; + if (typeof port !== 'number') { + return next(new restify.InvalidArgumentError( + {statusCode: 400}, + `Expected an numeric port, instead got ${port} of type ${typeof port}`)); + } + + await this.accessKeys.setPortForNewAccessKeys(port); + this.serverConfig.data().portForNewAccessKeys = port; + this.serverConfig.write(); + res.send(HttpSuccess.NO_CONTENT); + next(); + } catch (error) { + logging.error(error); + if (error instanceof errors.InvalidPortNumber) { + return next(new restify.InvalidArgumentError({statusCode: 400}, error.message)); + } else if (error instanceof errors.PortUnavailable) { + return next(new restify.ConflictError(error.message)); + } + return next(new restify.InternalServerError(error)); + } + } + // Removes an existing access key public removeAccessKey(req: RequestType, res: ResponseType, next: restify.Next): void { try { diff --git a/src/shadowbox/server/server_access_key.spec.ts b/src/shadowbox/server/server_access_key.spec.ts index 03737e1e8..60e5bd095 100644 --- a/src/shadowbox/server/server_access_key.spec.ts +++ b/src/shadowbox/server/server_access_key.spec.ts @@ -12,24 +12,26 @@ // See the License for the specific language governing permissions and // limitations under the License. +import * as net from 'net'; + import {ManualClock} from '../infrastructure/clock'; import {PortProvider} from '../infrastructure/get_port'; import {InMemoryConfig} from '../infrastructure/json_config'; import {AccessKeyQuota, AccessKeyRepository} from '../model/access_key'; +import * as errors from '../model/errors'; import {FakePrometheusClient, FakeShadowsocksServer} from './mocks/mocks'; import {AccessKeyConfigJson, ServerAccessKeyRepository} from './server_access_key'; -import {ServerConfigJson} from './server_config'; describe('ServerAccessKeyRepository', () => { it('Repos with non-existent files are created with no access keys', (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); expect(countAccessKeys(repo)).toEqual(0); done(); }); it('Can create new access keys', (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); repo.createNewAccessKey().then((accessKey) => { expect(accessKey).toBeDefined(); done(); @@ -37,7 +39,7 @@ describe('ServerAccessKeyRepository', () => { }); it('Creates access keys without quota and under quota', async (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); const accessKey = await repo.createNewAccessKey(); expect(accessKey.quotaUsage).toBeUndefined(); expect(accessKey.isOverQuota()).toBeFalsy(); @@ -45,7 +47,7 @@ describe('ServerAccessKeyRepository', () => { }); it('Can remove access keys', (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); repo.createNewAccessKey().then((accessKey) => { expect(countAccessKeys(repo)).toEqual(1); const removeResult = repo.removeAccessKey(accessKey.id); @@ -56,7 +58,7 @@ describe('ServerAccessKeyRepository', () => { }); it('removeAccessKey returns false for missing keys', (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); repo.createNewAccessKey().then((accessKey) => { expect(countAccessKeys(repo)).toEqual(1); const removeResult = repo.removeAccessKey('badId'); @@ -67,7 +69,7 @@ describe('ServerAccessKeyRepository', () => { }); it('Can rename access keys', (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); repo.createNewAccessKey().then((accessKey) => { const NEW_NAME = 'newName'; const renameResult = repo.renameAccessKey(accessKey.id, NEW_NAME); @@ -80,7 +82,7 @@ describe('ServerAccessKeyRepository', () => { }); it('renameAccessKey returns false for missing keys', (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); repo.createNewAccessKey().then((accessKey) => { const NEW_NAME = 'newName'; const renameResult = repo.renameAccessKey('badId', NEW_NAME); @@ -92,8 +94,105 @@ describe('ServerAccessKeyRepository', () => { }); }); + it('Creates keys at the right port by construction', async (done) => { + const portProvider = new PortProvider(); + const port = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().port(port).build(); + const key = await repo.createNewAccessKey(); + expect(key.proxyParams.portNumber).toEqual(port); + done(); + }); + + it('setPortForNewAccessKeys changes default port for new keys', async (done) => { + const portProvider = new PortProvider(); + const port = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().build(); + await repo.setPortForNewAccessKeys(port); + const key = await repo.createNewAccessKey(); + expect(key.proxyParams.portNumber).toEqual(port); + done(); + }); + + it('setPortForNewAccessKeys maintains ports on existing keys', async (done) => { + const portProvider = new PortProvider(); + const oldPort = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().port(oldPort).build(); + const oldKey = await repo.createNewAccessKey(); + + const newPort = await portProvider.reserveNewPort(); + await repo.setPortForNewAccessKeys(newPort); + expect(oldKey.proxyParams.portNumber).toEqual(oldPort); + done(); + }); + + it('setPortForNewAccessKeys rejects invalid port numbers', async (done) => { + const repo = new RepoBuilder().build(); + // jasmine.toThrowError expects a function and makes the code + // hard to read. + const expectThrow = async (port: number) => { + try { + await repo.setPortForNewAccessKeys(port); + fail(`setPortForNewAccessKeys should reject invalid port number ${port}.`); + } catch (error) { + expect(error instanceof errors.InvalidPortNumber).toBeTruthy(); + } + }; + await expectThrow(0); + await expectThrow(-1); + await expectThrow(100.1); + await expectThrow(65536); + done(); + }); + + it('setPortForNewAccessKeys rejects 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.setPortForNewAccessKeys(port); + fail(`setPortForNewAccessKeys should reject already used port ${port}.`); + } catch (error) { + expect(error instanceof errors.PortUnavailable); + } + server.close(); + done(); + }); + }); + + it('setPortForNewAccessKeys accepts ports already used by access keys', async (done) => { + const portProvider = new PortProvider(); + const oldPort = await portProvider.reserveNewPort(); + const repo = new RepoBuilder().port(oldPort).build(); + await repo.createNewAccessKey(); + + // jasmine.toThrowError expects a function and makes the code + // hard to read. We also can't do anything like + // `expect(repo.setPortForNewAccessKeys.bind(repo, port)).not.toThrow()` + // since setPortForNewAccessKeys is async and this would lead to false positives + // when expect() returns before setPortForNewAccessKeys throws. + const expectNoThrow = async (port: number) => { + try { + await repo.setPortForNewAccessKeys(port); + } catch (error) { + fail(`setPortForNewAccessKeys should accept port ${port}.`); + } + }; + + await expectNoThrow(await portProvider.reserveNewPort()); + + // simulate the first key's connection on its port + const server = new net.Server(); + server.listen(oldPort, async () => { + await expectNoThrow(oldPort); + server.close(); + done(); + }); + }); + it('Can set access key quota', async (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); const accessKey = await repo.createNewAccessKey(); const quota = {data: {bytes: 5000}, window: {hours: 24}}; expect(await repo.setAccessKeyQuota(accessKey.id, quota)).toBeTruthy(); @@ -104,7 +203,7 @@ describe('ServerAccessKeyRepository', () => { }); it('setAccessKeyQuota returns false for missing keys', async (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); await repo.createNewAccessKey(); const quota = {data: {bytes: 1000}, window: {hours: 24}}; expect(await repo.setAccessKeyQuota('doesnotexist', quota)).toBeFalsy(); @@ -112,7 +211,7 @@ describe('ServerAccessKeyRepository', () => { }); it('setAccessKeyQuota fails with disallowed quota values', async (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); const accessKey = await repo.createNewAccessKey(); // Negative values const negativeBytesQuota = {data: {bytes: -1000}, window: {hours: 24}}; @@ -132,10 +231,9 @@ describe('ServerAccessKeyRepository', () => { it('setAccessKeyQuota updates keys quota status', async (done) => { const server = new FakeShadowsocksServer(); const prometheusClient = new FakePrometheusClient({'0': 500, '1': 200}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), server, - prometheusClient); + const repo = + new RepoBuilder().prometheusClient(prometheusClient).shadowsocksServer(server).build(); + const accessKey1 = await repo.createNewAccessKey(); const accessKey2 = await repo.createNewAccessKey(); @@ -163,7 +261,7 @@ describe('ServerAccessKeyRepository', () => { }); it('can remove access key quotas', async (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); const accessKey = await repo.createNewAccessKey(); await expect(repo.setAccessKeyQuota(accessKey.id, {data: {bytes: 100}, window: {hours: 24}})) .toBeTruthy(); @@ -174,7 +272,7 @@ describe('ServerAccessKeyRepository', () => { }); it('removeAccessKeyQuota returns false for missing keys', async (done) => { - const repo = createRepo(); + const repo = new RepoBuilder().build(); await repo.createNewAccessKey(); expect(await repo.removeAccessKeyQuota('doesnotexist')).toBeFalsy(); done(); @@ -183,10 +281,9 @@ describe('ServerAccessKeyRepository', () => { it('removeAccessKeyQuota restores over-quota access keys when removing quota ', async (done) => { const server = new FakeShadowsocksServer(); const prometheusClient = new FakePrometheusClient({'0': 500, '1': 100}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), server, - prometheusClient); + const repo = + new RepoBuilder().prometheusClient(prometheusClient).shadowsocksServer(server).build(); + const accessKey = await repo.createNewAccessKey(); await repo.createNewAccessKey(); await repo.setAccessKeyQuota(accessKey.id, {data: {bytes: 100}, window: {hours: 1}}); @@ -205,10 +302,8 @@ describe('ServerAccessKeyRepository', () => { it('enforceAccessKeyQuotas updates keys quota status ', async (done) => { const prometheusClient = new FakePrometheusClient({'0': 500, '1': 100}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), - new FakeShadowsocksServer(), prometheusClient); + const repo = new RepoBuilder().prometheusClient(prometheusClient).build(); + const accessKey1 = await repo.createNewAccessKey(); await repo.createNewAccessKey(); await repo.setAccessKeyQuota(accessKey1.id, {data: {bytes: 200}, window: {hours: 1}}); @@ -233,10 +328,9 @@ describe('ServerAccessKeyRepository', () => { it('enforceAccessKeyQuotas enables and disables keys', async (done) => { const server = new FakeShadowsocksServer(); const prometheusClient = new FakePrometheusClient({'0': 500, '1': 100}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), server, - prometheusClient); + const repo = + new RepoBuilder().prometheusClient(prometheusClient).shadowsocksServer(server).build(); + const accessKey1 = await repo.createNewAccessKey(); const accessKey2 = await repo.createNewAccessKey(); await repo.setAccessKeyQuota(accessKey1.id, {data: {bytes: 200}, window: {hours: 1}}); @@ -256,9 +350,7 @@ describe('ServerAccessKeyRepository', () => { it('Repos created with an existing file restore access keys', async (done) => { const config = new InMemoryConfig({accessKeys: [], nextId: 0}); - const repo1 = new ServerAccessKeyRepository( - new PortProvider(), 'hostname', config, new FakeShadowsocksServer(), - new FakePrometheusClient({})); + const repo1 = new RepoBuilder().keyConfig(config).build(); // Create 2 new access keys await Promise.all([repo1.createNewAccessKey(), repo1.createNewAccessKey()]); // Modify properties @@ -267,9 +359,7 @@ describe('ServerAccessKeyRepository', () => { // Create a 2nd repo from the same config file. This simulates what // might happen after the shadowbox server is restarted. - const repo2 = new ServerAccessKeyRepository( - new PortProvider(), 'hostname', config, new FakeShadowsocksServer(), - new FakePrometheusClient({})); + const repo2 = new RepoBuilder().keyConfig(config).build(); // Check that repo1 and repo2 have the same access keys expect(repo1.listAccessKeys()).toEqual(repo2.listAccessKeys()); done(); @@ -278,16 +368,13 @@ describe('ServerAccessKeyRepository', () => { it('Does not re-use ids when using the same config file', (done) => { const config = new InMemoryConfig({accessKeys: [], nextId: 0}); // Create a repo with 1 access key, then delete that access key. - const repo1 = new ServerAccessKeyRepository( - new PortProvider(), '', config, new FakeShadowsocksServer(), new FakePrometheusClient({})); + const repo1 = new RepoBuilder().keyConfig(config).build(); repo1.createNewAccessKey().then((accessKey1) => { repo1.removeAccessKey(accessKey1.id); // Create a 2nd repo with one access key, and verify that // it hasn't reused the first access key's ID. - const repo2 = new ServerAccessKeyRepository( - new PortProvider(), '', config, new FakeShadowsocksServer(), - new FakePrometheusClient({})); + const repo2 = new RepoBuilder().keyConfig(config).build(); repo2.createNewAccessKey().then((accessKey2) => { expect(accessKey1.id).not.toEqual(accessKey2.id); done(); @@ -297,15 +384,14 @@ describe('ServerAccessKeyRepository', () => { it('start exposes the access keys to the server', async (done) => { const config = new InMemoryConfig({accessKeys: [], nextId: 0}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', config, new FakeShadowsocksServer(), new FakePrometheusClient({})); + const repo = new RepoBuilder().keyConfig(config).build(); + const accessKey1 = await repo.createNewAccessKey(); const accessKey2 = await repo.createNewAccessKey(); // Create a new repository with the same configuration. The keys should not be exposed to the // server until `start` is called. const server = new FakeShadowsocksServer(); - const repo2 = new ServerAccessKeyRepository( - new PortProvider(), '', config, server, new FakePrometheusClient({})); + const repo2 = new RepoBuilder().keyConfig(config).shadowsocksServer(server).build(); expect(server.getAccessKeys().length).toEqual(0); await repo2.start(new ManualClock()); const serverAccessKeys = server.getAccessKeys(); @@ -318,10 +404,9 @@ describe('ServerAccessKeyRepository', () => { it('start periodically enforces access key quotas', async (done) => { const server = new FakeShadowsocksServer(); const prometheusClient = new FakePrometheusClient({'0': 500, '1': 300, '2': 1000}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), server, - prometheusClient); + const repo = + new RepoBuilder().prometheusClient(prometheusClient).shadowsocksServer(server).build(); + const accessKey1 = await repo.createNewAccessKey(); const accessKey2 = await repo.createNewAccessKey(); const accessKey3 = await repo.createNewAccessKey(); @@ -361,10 +446,7 @@ describe('ServerAccessKeyRepository', () => { it('getOutboundByteTransfer', async (done) => { const prometheusClient = new FakePrometheusClient({'0': 1024}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), - new FakeShadowsocksServer(), prometheusClient); + const repo = new RepoBuilder().prometheusClient(prometheusClient).build(); const bytesTransferred = await repo.getOutboundByteTransfer('0', 10); expect(bytesTransferred).toEqual(1024); done(); @@ -372,10 +454,7 @@ describe('ServerAccessKeyRepository', () => { it('getOutboundByteTransfer returns zero when ID is missing', async (done) => { const prometheusClient = new FakePrometheusClient({'0': 1024}); - const repo = new ServerAccessKeyRepository( - new PortProvider(), '', - new InMemoryConfig({accessKeys: [], nextId: 0}), - new FakeShadowsocksServer(), prometheusClient); + const repo = new RepoBuilder().prometheusClient(prometheusClient).build(); const bytesTransferred = await repo.getOutboundByteTransfer('doesnotexist', 10); expect(bytesTransferred).toEqual(0); done(); @@ -386,9 +465,31 @@ function countAccessKeys(repo: AccessKeyRepository) { return repo.listAccessKeys().length; } -function createRepo(): ServerAccessKeyRepository { - const config = new InMemoryConfig({accessKeys: [], nextId: 0}); - return new ServerAccessKeyRepository( - new PortProvider(), 'hostname', config, new FakeShadowsocksServer(), - new FakePrometheusClient({})); +class RepoBuilder { + private port_ = 12345; + private keyConfig_ = new InMemoryConfig({accessKeys: [], nextId: 0}); + private shadowsocksServer_ = new FakeShadowsocksServer(); + private prometheusClient_ = new FakePrometheusClient({}); + + public port(port: number): RepoBuilder { + this.port_ = port; + return this; + } + public keyConfig(keyConfig: InMemoryConfig): RepoBuilder { + this.keyConfig_ = keyConfig; + return this; + } + public shadowsocksServer(shadowsocksServer: FakeShadowsocksServer): RepoBuilder { + this.shadowsocksServer_ = shadowsocksServer; + return this; + } + public prometheusClient(prometheusClient: FakePrometheusClient): RepoBuilder { + this.prometheusClient_ = prometheusClient; + return this; + } + + public build(): ServerAccessKeyRepository { + return new ServerAccessKeyRepository( + this.port_, 'hostname', this.keyConfig_, this.shadowsocksServer_, this.prometheusClient_); + } } diff --git a/src/shadowbox/server/server_access_key.ts b/src/shadowbox/server/server_access_key.ts index 8fe81a133..38d8d0c9d 100644 --- a/src/shadowbox/server/server_access_key.ts +++ b/src/shadowbox/server/server_access_key.ts @@ -16,15 +16,13 @@ import * as randomstring from 'randomstring'; import * as uuidv4 from 'uuid/v4'; import {Clock} from '../infrastructure/clock'; -import {PortProvider} from '../infrastructure/get_port'; +import {isPortUsed} from '../infrastructure/get_port'; import {JsonConfig} from '../infrastructure/json_config'; import * as logging from '../infrastructure/logging'; import {PrometheusClient} from '../infrastructure/prometheus_scraper'; import {AccessKey, AccessKeyId, AccessKeyMetricsId, AccessKeyQuota, AccessKeyQuotaUsage, AccessKeyRepository, ProxyParams} from '../model/access_key'; -import {ShadowsocksAccessKey, ShadowsocksServer} from '../model/shadowsocks_server'; - -import {ManagerMetrics} from './manager_metrics'; -import {ServerConfigJson} from './server_config'; +import * as errors from '../model/errors'; +import {ShadowsocksServer} from '../model/shadowsocks_server'; // The format as json of access keys in the config file. interface AccessKeyJson { @@ -42,9 +40,6 @@ export interface AccessKeyConfigJson { accessKeys?: AccessKeyJson[]; // Next AccessKeyId to use. nextId?: number; - - // DEPRECATED: Use ServerConfigJson.portForNewAccessKeys instead. - defaultPort?: number; } // AccessKey implementation with write access enabled on properties that may change. @@ -92,15 +87,15 @@ function makeAccessKeyJson(accessKey: AccessKey): AccessKeyJson { } // AccessKeyRepository that keeps its state in a config file and uses ShadowsocksServer -// to start and stop per-access-key Shadowsocks instances. +// to start and stop per-access-key Shadowsocks instances. Requires external validation +// that portForNewAccessKeys is valid. export class ServerAccessKeyRepository implements AccessKeyRepository { private static QUOTA_ENFORCEMENT_INTERVAL_MS = 60 * 60 * 1000; // 1h private NEW_USER_ENCRYPTION_METHOD = 'chacha20-ietf-poly1305'; private accessKeys: ServerAccessKey[]; - private portForNewAccessKeys: number|undefined; constructor( - private portProvider: PortProvider, private proxyHostname: string, + private portForNewAccessKeys: number, private proxyHostname: string, private keyConfig: JsonConfig, private shadowsocksServer: ShadowsocksServer, private prometheusClient: PrometheusClient) { if (this.keyConfig.data().accessKeys === undefined) { @@ -126,19 +121,30 @@ export class ServerAccessKeyRepository implements AccessKeyRepository { }, ServerAccessKeyRepository.QUOTA_ENFORCEMENT_INTERVAL_MS); } - enableSinglePort(portForNewAccessKeys: number) { - this.portForNewAccessKeys = portForNewAccessKeys; + private isExistingAccessKeyPort(port: number): boolean { + return this.accessKeys.some((key) => { + return key.proxyParams.portNumber === port; + }); + } + + async setPortForNewAccessKeys(port: number): Promise { + if (!Number.isInteger(port) || port < 1 || port > 65535) { + throw new errors.InvalidPortNumber(port.toString()); + } + if (!this.isExistingAccessKeyPort(port) && await isPortUsed(port)) { + throw new errors.PortUnavailable(port); + } + this.portForNewAccessKeys = port; } async createNewAccessKey(): Promise { - const port = this.portForNewAccessKeys || await this.portProvider.reserveNewPort(); const id = this.keyConfig.data().nextId.toString(); this.keyConfig.data().nextId += 1; const metricsId = uuidv4(); const password = generatePassword(); const proxyParams = { hostname: this.proxyHostname, - portNumber: port, + portNumber: this.portForNewAccessKeys, encryptionMethod: this.NEW_USER_ENCRYPTION_METHOD, password, }; @@ -153,7 +159,6 @@ export class ServerAccessKeyRepository implements AccessKeyRepository { for (let ai = 0; ai < this.accessKeys.length; ai++) { const accessKey = this.accessKeys[ai]; if (accessKey.id === id) { - this.portProvider.freePort(accessKey.proxyParams.portNumber); this.accessKeys.splice(ai, 1); this.saveAccessKeys(); this.updateServer();