Create an endpoint to change the default port for new access keys (Ji…

…gsaw-Code#461)

* Add an endpoint to set a new port for access keys

* post commit formatting

* Document the new api

* Remove enum in errors

* Respond to review comments

* Respond to review comments

* Rename to ShadowboxError and add copywright header

* Move to errors.ts

* Respond to coments in access_key.ts

* Move to errors.ts

* More renames

* post commit formatting

* More review responses

* Correctly convert invalid port number to string

* More renaming

* post commit formatting

* Respond to review comments

* Fix issue using parseInt

* Change endpoint URL

* formatting

* Respond to more review comments

* Respond to comments, add to integration test

* post commit formatting

* Move invalidPortArgument into a local

* formatting

* More review

* more review

* formatting

* disambiguate errors using restify types

* formatting

* fix the integration test

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<net.Server> {

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<boolean> {
+export function isPortUsed(port: number): Promise<boolean> {
   return new Promise((resolve, reject) => {
     let isUsed = false;
     const server = new net.Server();

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

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<void>;
   // Apply the specified update to the specified access key.
   // Returns true if successful.
   renameAccessKey(id: AccessKeyId, name: string): boolean;

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());
+  }
+}

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

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<AccessKeyConfigJson>, 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<server_config.ServerConfigJson>,
-    keyConfig: json_config.JsonConfig<AccessKeyConfigJson>): 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<server_config.ServerConfigJson>): Promise<number> {
-  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<AccessKeyConfigJson>(
       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}`);
   });