fix: ReverseConnection waits for incoming data before piping data back

There was an issue in obscure conditions that caused certain packets to be out of the expected order for HTTP2 leading to a protocol error. This ensure that the `MAGIC` frame comes before the server's `SETTING` frame. Updated `NodeConnectionManager` lifecycle logging. `Proxy` and `NodeConnection` rejects connections to wildcard `0.0.0.0` and `::` addresses. Fixed up any tests broken by this change. Added a test to check if you can connect to the target through the proxies after the proxies have created the UTP connection. Fixes #369
MatrixAI · Apr 7, 2022 · 9e44d52 · 9e44d52
1 parent db3b913
commit 9e44d52
Show file tree

Hide file tree

Showing 18 changed files with 199 additions and 10 deletions.
diff --git a/src/network/ConnectionReverse.ts b/src/network/ConnectionReverse.ts
@@ -295,7 +295,9 @@ class ConnectionReverse extends Connection {
         await this.stop();
       });
       this.tlsSocket.pipe(this.serverSocket, { end: false });
-      this.serverSocket.pipe(this.tlsSocket, { end: false });
+      this.tlsSocket.once('data', () => {
+        this.serverSocket.pipe(this.tlsSocket as TLSSocket, { end: false });
+      });
       this.clientCertChain = clientCertChain;
       this.logger.info('Composed Connection Reverse');
     } catch (e) {

diff --git a/src/network/Proxy.ts b/src/network/Proxy.ts
@@ -434,6 +434,13 @@ class Proxy {
           timer,
         );
       } catch (e) {
+        if (e instanceof networkErrors.ErrorProxyConnectInvalidUrl) {
+          if (!clientSocket.destroyed) {
+            await clientSocketEnd('HTTP/1.1 400 Bad Request\r\n' + '\r\n');
+            clientSocket.destroy(e);
+          }
+          return;
+        }
         if (e instanceof networkErrors.ErrorConnectionStartTimeout) {
           if (!clientSocket.destroyed) {
             await clientSocketEnd('HTTP/1.1 504 Gateway Timeout\r\n' + '\r\n');
@@ -519,6 +526,9 @@ class Proxy {
     proxyPort: Port,
     timer?: Timer,
   ): Promise<ConnectionForward> {
+    if (networkUtils.isHostWildcard(proxyHost)) {
+      throw new networkErrors.ErrorProxyConnectInvalidUrl();
+    }
     const proxyAddress = networkUtils.buildAddress(proxyHost, proxyPort);
     let conn: ConnectionForward | undefined;
     conn = this.connectionsForward.proxy.get(proxyAddress);
@@ -681,6 +691,9 @@ class Proxy {
     proxyPort: Port,
     timer?: Timer,
   ): Promise<ConnectionReverse> {
+    if (networkUtils.isHostWildcard(proxyHost)) {
+      throw new networkErrors.ErrorProxyConnectInvalidUrl();
+    }
     const proxyAddress = networkUtils.buildAddress(proxyHost, proxyPort);
     let conn = this.connectionsReverse.proxy.get(proxyAddress);
     if (conn != null) {

diff --git a/src/network/utils.ts b/src/network/utils.ts
@@ -29,6 +29,10 @@ function isHost(host: any): host is Host {
   return isIPv4 || isIPv6;
 }
 
+function isHostWildcard(host: Host): boolean {
+  return host === '0.0.0.0' || host === '::';
+}
+
 /**
  * Validates hostname as per RFC 1123
  */
@@ -353,6 +357,7 @@ export {
   pingBuffer,
   pongBuffer,
   isHost,
+  isHostWildcard,
   isHostname,
   isPort,
   toAuthToken,

diff --git a/src/nodes/NodeConnection.ts b/src/nodes/NodeConnection.ts
@@ -59,6 +59,10 @@ class NodeConnection<T extends GRPCClient> {
     logger?: Logger;
   }): Promise<NodeConnection<T>> {
     logger.info(`Creating ${this.name}`);
+    // Checking if attempting to connect to a wildcard IP
+    if (networkUtils.isHostWildcard(targetHost)) {
+      throw new nodesErrors.ErrorNodeConnectionHostWildcard();
+    }
     const proxyConfig = {
       host: proxy.getForwardHost(),
       port: proxy.getForwardPort(),

diff --git a/src/nodes/NodeConnectionManager.ts b/src/nodes/NodeConnectionManager.ts
@@ -129,7 +129,7 @@ class NodeConnectionManager {
     targetNodeId: NodeId,
   ): Promise<ResourceAcquire<NodeConnection<GRPCClientAgent>>> {
     return async () => {
-      const connAndLock = await this.createConnection(targetNodeId);
+      const connAndLock = await this.getConnection(targetNodeId);
       // Acquire the read lock and the release function
       const release = await connAndLock.lock.acquireRead();
       // Resetting TTL timer
@@ -161,6 +161,11 @@ class NodeConnectionManager {
       return await withF(
         [await this.acquireConnection(targetNodeId)],
         async ([conn]) => {
+          this.logger.info(
+            `withConnF calling function with connection to ${nodesUtils.encodeNodeId(
+              targetNodeId,
+            )}`,
+          );
           return await f(conn);
         },
       );
@@ -219,11 +224,11 @@ class NodeConnectionManager {
    * @param targetNodeId Id of node we are creating connection to
    * @returns ConnectionAndLock that was create or exists in the connection map.
    */
-  protected async createConnection(
+  protected async getConnection(
     targetNodeId: NodeId,
   ): Promise<ConnectionAndLock> {
     this.logger.info(
-      `Creating connection to ${nodesUtils.encodeNodeId(targetNodeId)}`,
+      `Getting connection to ${nodesUtils.encodeNodeId(targetNodeId)}`,
     );
     let connection: NodeConnection<GRPCClientAgent> | undefined;
     let lock: RWLock;
@@ -241,10 +246,13 @@ class NodeConnectionManager {
           targetNodeId.toString() as NodeIdString,
         );
         if (connAndLock != null && connAndLock.connection != null) {
+          this.logger.info(
+            `existing entry found for ${nodesUtils.encodeNodeId(targetNodeId)}`,
+          );
           return connAndLock;
         }
         this.logger.info(
-          `existing lock: creating connection to ${nodesUtils.encodeNodeId(
+          `existing lock, creating connection to ${nodesUtils.encodeNodeId(
             targetNodeId,
           )}`,
         );
@@ -260,7 +268,7 @@ class NodeConnectionManager {
       );
       return await lock.withWrite(async () => {
         this.logger.info(
-          `no existing entry: creating connection to ${nodesUtils.encodeNodeId(
+          `no existing entry, creating connection to ${nodesUtils.encodeNodeId(
             targetNodeId,
           )}`,
         );
@@ -318,7 +326,9 @@ class NodeConnectionManager {
       nodeConnectionManager: this,
       destroyCallback,
       connConnectTime: this.connConnectTime,
-      logger: this.logger.getChild(`${targetHost}:${targetAddress.port}`),
+      logger: this.logger.getChild(
+        `${NodeConnection.name} ${targetHost}:${targetAddress.port}`,
+      ),
       clientFactory: async (args) =>
         GRPCClientAgent.createGRPCClientAgent(args),
     });
@@ -521,7 +531,7 @@ class NodeConnectionManager {
         // Add the node to the database so that we can find its address in
         // call to getConnectionToNode
         await this.nodeGraph.setNode(nextNode.id, nextNode.address);
-        await this.createConnection(nextNode.id);
+        await this.getConnection(nextNode.id);
       } catch (e) {
         // If we can't connect to the node, then skip it
         continue;
@@ -620,7 +630,7 @@ class NodeConnectionManager {
     for (const seedNodeId of this.getSeedNodes()) {
       // Check if the connection is viable
       try {
-        await this.createConnection(seedNodeId);
+        await this.getConnection(seedNodeId);
       } catch (e) {
         if (e instanceof nodesErrors.ErrorNodeConnectionTimeout) continue;
         throw e;

diff --git a/src/nodes/errors.ts b/src/nodes/errors.ts
@@ -62,6 +62,11 @@ class ErrorNodeConnectionManagerNotRunning extends ErrorNodes {
   exitCode = sysexits.USAGE;
 }
 
+class ErrorNodeConnectionHostWildcard extends ErrorNodes {
+  description = 'An IP wildcard was provided for the target host';
+  exitCode = sysexits.USAGE;
+}
+
 export {
   ErrorNodes,
   ErrorNodeGraphRunning,
@@ -76,4 +81,5 @@ export {
   ErrorNodeConnectionInfoNotExist,
   ErrorNodeConnectionPublicKeyNotFound,
   ErrorNodeConnectionManagerNotRunning,
+  ErrorNodeConnectionHostWildcard,
 };
diff --git a/tests/agent/service/nodesCrossSignClaim.test.ts b/tests/agent/service/nodesCrossSignClaim.test.ts
@@ -52,6 +52,9 @@ describe('nodesCrossSignClaim', () => {
         rootKeyPairBits: 2048,
       },
       seedNodes: {}, // Explicitly no seed nodes on startup
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger,
     });
     localId = pkAgent.keyManager.getNodeId();
@@ -63,6 +66,9 @@ describe('nodesCrossSignClaim', () => {
         rootKeyPairBits: 2048,
       },
       seedNodes: {}, // Explicitly no seed nodes on startup
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger,
     });
     remoteId = remoteNode.keyManager.getNodeId();

diff --git a/tests/bin/vaults/vaults.test.ts b/tests/bin/vaults/vaults.test.ts
@@ -1,5 +1,6 @@
 import type { NodeIdEncoded, NodeAddress, NodeInfo } from '@/nodes/types';
 import type { VaultId, VaultName } from '@/vaults/types';
+import type { Host } from '@/network/types';
 import os from 'os';
 import path from 'path';
 import fs from 'fs';
@@ -207,6 +208,9 @@ describe('CLI vaults', () => {
       const targetPolykeyAgent = await PolykeyAgent.createPolykeyAgent({
         password,
         nodePath: dataDir2,
+        networkConfig: {
+          proxyHost: '127.0.0.1' as Host,
+        },
         logger: logger,
       });
       const vaultId = await targetPolykeyAgent.vaultManager.createVault(
@@ -701,6 +705,9 @@ describe('CLI vaults', () => {
             password,
             logger,
             nodePath: path.join(dataDir, 'remoteOnline'),
+            networkConfig: {
+              proxyHost: '127.0.0.1' as Host,
+            },
           });
           const remoteOnlineNodeId = remoteOnline.keyManager.getNodeId();
           const remoteOnlineNodeIdEncoded =

diff --git a/tests/network/Proxy.test.ts b/tests/network/Proxy.test.ts
@@ -208,6 +208,15 @@ describe(Proxy.name, () => {
         `127.0.0.1:80?nodeId=${encodeURIComponent(nodeIdSomeEncoded)}`,
       ),
     ).rejects.toThrow('407');
+    // Wildcard as host
+    await expect(() =>
+      httpConnect(
+        proxy.getForwardHost(),
+        proxy.getForwardPort(),
+        authToken,
+        `0.0.0.0:80?nodeId=${encodeURIComponent(nodeIdSomeEncoded)}`,
+      ),
+    ).rejects.toThrow('400');
     // No node id
     await expect(() =>
       httpConnect(

diff --git a/tests/nodes/NodeConnection.test.ts b/tests/nodes/NodeConnection.test.ts
@@ -433,6 +433,47 @@ describe(`${NodeConnection.name} test`, () => {
     });
     await conn.destroy();
   });
+  test('connects to its target but proxies connect first', async () => {
+    await clientproxy.openConnectionForward(
+      targetNodeId,
+      localHost,
+      targetPort,
+    );
+    const conn = await NodeConnection.createNodeConnection({
+      targetNodeId: targetNodeId,
+      targetHost: localHost,
+      targetPort: targetPort,
+      proxy: clientproxy,
+      keyManager: clientKeyManager,
+      nodeConnectionManager: dummyNodeConnectionManager,
+      destroyCallback,
+      logger: logger,
+      clientFactory: async (args) =>
+        GRPCClientAgent.createGRPCClientAgent(args),
+    });
+    // Because the connection will not have enough time to compose before we
+    // attempt to acquire the connection info, we need to wait and poll it
+    const connInfo = await poll<ConnectionInfo | undefined>(
+      async () => {
+        return serverProxy.getConnectionInfoByProxy(localHost, sourcePort);
+      },
+      (e) => {
+        if (e instanceof networkErrors.ErrorConnectionNotComposed) return false;
+        if (e instanceof networkErrors.ErrorConnectionNotRunning) return false;
+        return true;
+      },
+    );
+    expect(connInfo).toBeDefined();
+    expect(connInfo).toMatchObject({
+      remoteNodeId: sourceNodeId,
+      remoteCertificates: expect.any(Array),
+      localHost: localHost,
+      localPort: targetPort,
+      remoteHost: localHost,
+      remotePort: sourcePort,
+    });
+    await conn.destroy();
+  });
   test('grpcCall after connection drops', async () => {
     let nodeConnection: NodeConnection<GRPCClientAgent> | undefined;
     let polykeyAgent: PolykeyAgent | undefined;
@@ -441,6 +482,9 @@ describe(`${NodeConnection.name} test`, () => {
         password,
         nodePath: path.join(dataDir, 'PolykeyAgent3'),
         logger: logger,
+        networkConfig: {
+          proxyHost: localHost,
+        },
       });
       // Have a nodeConnection try to connect to it
       const killSelf = jest.fn();
@@ -629,6 +673,9 @@ describe(`${NodeConnection.name} test`, () => {
         password,
         nodePath: path.join(dataDir, 'PolykeyAgent3'),
         logger: logger,
+        networkConfig: {
+          proxyHost: localHost,
+        },
       });
       // Have a nodeConnection try to connect to it
       const killSelf = jest.fn();

diff --git a/tests/nodes/NodeConnectionManager.general.test.ts b/tests/nodes/NodeConnectionManager.general.test.ts
@@ -139,12 +139,18 @@ describe(`${NodeConnectionManager.name} general test`, () => {
     remoteNode1 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode1'),
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger: logger.getChild('remoteNode1'),
     });
     remoteNodeId1 = remoteNode1.keyManager.getNodeId();
     remoteNode2 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode2'),
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger: logger.getChild('remoteNode2'),
     });
     remoteNodeId2 = remoteNode2.keyManager.getNodeId();
@@ -266,6 +272,9 @@ describe(`${NodeConnectionManager.name} general test`, () => {
         const server = await PolykeyAgent.createPolykeyAgent({
           nodePath: path.join(dataDir, 'node2'),
           password,
+          networkConfig: {
+            proxyHost: '127.0.0.1' as Host,
+          },
           logger: nodeConnectionManagerLogger,
         });
         await nodeGraph.setNode(server.keyManager.getNodeId(), {
@@ -300,6 +309,9 @@ describe(`${NodeConnectionManager.name} general test`, () => {
         const server = await PolykeyAgent.createPolykeyAgent({
           nodePath: path.join(dataDir, 'node3'),
           password,
+          networkConfig: {
+            proxyHost: '127.0.0.1' as Host,
+          },
           logger: nodeConnectionManagerLogger,
         });
         await nodeGraph.setNode(server.keyManager.getNodeId(), {
@@ -456,6 +468,9 @@ describe(`${NodeConnectionManager.name} general test`, () => {
         password,
         logger: logger.getChild('serverPKAgent'),
         nodePath: path.join(dataDir, 'serverPKAgent'),
+        networkConfig: {
+          proxyHost: '127.0.0.1' as Host,
+        },
       });
       nodeConnectionManager = new NodeConnectionManager({
         keyManager,

diff --git a/tests/nodes/NodeConnectionManager.lifecycle.test.ts b/tests/nodes/NodeConnectionManager.lifecycle.test.ts
@@ -97,12 +97,18 @@ describe(`${NodeConnectionManager.name} lifecycle test`, () => {
     remoteNode1 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode1'),
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger: logger.getChild('remoteNode1'),
     });
     remoteNodeId1 = remoteNode1.keyManager.getNodeId();
     remoteNode2 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode2'),
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger: logger.getChild('remoteNode2'),
     });
     remoteNodeId2 = remoteNode2.keyManager.getNodeId();

diff --git a/tests/nodes/NodeConnectionManager.seednodes.test.ts b/tests/nodes/NodeConnectionManager.seednodes.test.ts
@@ -90,12 +90,18 @@ describe(`${NodeConnectionManager.name} seed nodes test`, () => {
     remoteNode1 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode1'),
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger: logger.getChild('remoteNode1'),
     });
     remoteNodeId1 = remoteNode1.keyManager.getNodeId();
     remoteNode2 = await PolykeyAgent.createPolykeyAgent({
       password,
       nodePath: path.join(dataDir2, 'remoteNode2'),
+      networkConfig: {
+        proxyHost: '127.0.0.1' as Host,
+      },
       logger: logger.getChild('remoteNode2'),
     });
     remoteNodeId2 = remoteNode2.keyManager.getNodeId();