wip: added authentication timeout and other fixes

Author: tegefaulkes

src/nodes/NodeManager.ts

@@ -7,9 +7,9 @@ import type Sigchain from '../sigchain/Sigchain';
 import type TaskManager from '../tasks/TaskManager';
 import type GestaltGraph from '../gestalts/GestaltGraph';
 import type {
+  Task,
   TaskHandler,
   TaskHandlerId,
-  Task,
   TaskInfo,
 } from '../tasks/types';
 import type { SignedTokenEncoded } from '../tokens/types';
@@ -23,32 +23,33 @@ import type {
 import type { ClaimLinkNode } from '../claims/payloads';
 import type NodeConnection from '../nodes/NodeConnection';
 import type {
+  AgentClaimMessage,
   AgentRPCRequestParams,
   AgentRPCResponseResult,
-  AgentClaimMessage,
   NodesAuthenticateConnectionMessage,
 } from './agent/types';
 import type {
-  NodeId,
+  AuthenticateNetworkForwardCallback,
+  AuthenticateNetworkReverseCallback,
   NodeAddress,
   NodeBucket,
   NodeBucketIndex,
   NodeContactAddressData,
+  NodeId,
   NodeIdEncoded,
-  AuthenticateNetworkForwardCallback,
-  AuthenticateNetworkReverseCallback,
   NodeIdString,
 } from './types';
 import type NodeConnectionManager from './NodeConnectionManager';
 import type NodeGraph from './NodeGraph';
 import type { ServicePOJO } from '@matrixai/mdns';
+import { withF } from '@matrixai/resources';
+import { events as mdnsEvents, MDNS, utils as mdnsUtils } from '@matrixai/mdns';
 import Logger from '@matrixai/logger';
-import { StartStop, ready } from '@matrixai/async-init/dist/StartStop';
-import { Semaphore, Lock } from '@matrixai/async-locks';
+import { ready, StartStop } from '@matrixai/async-init/dist/StartStop';
+import { Lock, Semaphore } from '@matrixai/async-locks';
 import { IdInternal } from '@matrixai/id';
-import { timedCancellable, context } from '@matrixai/contexts/dist/decorators';
-import { withF } from '@matrixai/resources';
-import { MDNS, events as mdnsEvents, utils as mdnsUtils } from '@matrixai/mdns';
+import { context, timedCancellable } from '@matrixai/contexts/dist/decorators';
+import { Timer } from '@matrixai/timer';
 import * as nodesUtils from './utils';
 import * as nodesEvents from './events';
 import * as nodesErrors from './errors';
@@ -77,12 +78,13 @@ enum AuthenticatingState {
 }
 type AuthenticationEntry = {
   authenticatedForward: AuthenticatingState;
-  reasonForward?: any;
+  reasonForward?: Error;
   authenticatedReverse: AuthenticatingState;
-  reasonReverse?: any;
+  reasonReverse?: Error;
   authenticatedP: Promise<void>;
   authenticatedResolveP: (value: void) => void;
-  authenticatedRejectP: (reason?: any) => void;
+  authenticatedRejectP: (reason?: Error) => void;
+  timeout?: Timer;
 };
 
 const activeForwardAuthenticateCancellationReason = Symbol(
@@ -1182,10 +1184,14 @@ class NodeManager {
       });
       connectionsEntry.authenticatedForward = AuthenticatingState.SUCCESS;
     } catch (e) {
+      const err = new nodesErrors.ErrorNodeManagerForwardAuthenticationFailed(
+        undefined,
+        { cause: e },
+      );
       connectionsEntry.authenticatedForward = AuthenticatingState.FAIL;
-      connectionsEntry.reasonForward = e;
+      connectionsEntry.reasonForward = err;
       await this.authenticateFail(nodeId);
-      throw e;
+      return;
     }
     this.logger.warn(
       `Node ${nodesUtils.encodeNodeId(nodeId)} has been forward authenticated`,
@@ -1270,9 +1276,6 @@ class NodeManager {
     }
   }
 
-  // TODO: There's a race condition with this and awaiting authentication. There are cases where authentication can complete before we can even start awaiting authenctication. We need to handle this better.
-  //  It's better handled by starting the wait for authentication and then initiating it but the ordering of this sucks.
-  //  best thing for now is to allow awaiting the authentication before even triggering the connections.
   /**
    * Will initiate a forward authentication call and coalesce
    */
@@ -1286,18 +1289,61 @@ class NodeManager {
         resolveP: authenticatedResolveP,
         rejectP: authenticatedRejectP,
       } = utils.promise<void>();
-      // Prevent unhandled rejections
-      authenticatedP.then(
-        () => {},
-        () => {},
-      );
+      const timeoutDelay = 1000;
+      const timeout = new Timer({ delay: timeoutDelay });
+      // Prevent cancellation leak when cleaning up
+      void timeout.catch(() => {});
       authenticationEntry = {
         authenticatedForward: AuthenticatingState.PENDING,
         authenticatedReverse: AuthenticatingState.PENDING,
         authenticatedP,
         authenticatedResolveP,
         authenticatedRejectP,
+        timeout,
       };
+      void timeout.then(
+        async () => {
+          this.logger.warn(`TIMED OUT!`);
+          const error = new nodesErrors.ErrorNodeManagerAuthenticatonTimedOut(
+            `Authentication timed out after ${timeoutDelay}ms`,
+          );
+          if (
+            authenticationEntry!.authenticatedForward ===
+            AuthenticatingState.PENDING
+          ) {
+            authenticationEntry!.authenticatedForward =
+              AuthenticatingState.FAIL;
+            authenticationEntry!.reasonForward = error;
+          }
+          if (
+            authenticationEntry!.authenticatedReverse ===
+            AuthenticatingState.PENDING
+          ) {
+            authenticationEntry!.authenticatedReverse =
+              AuthenticatingState.FAIL;
+            authenticationEntry!.reasonReverse = error;
+          }
+          if (
+            authenticationEntry!.authenticatedForward ===
+              AuthenticatingState.FAIL ||
+            authenticationEntry!.authenticatedReverse ===
+              AuthenticatingState.FAIL
+          ) {
+            await this.authenticateFail(nodeId);
+          }
+          delete authenticationEntry!.timeout;
+        },
+        () => {},
+      );
+      const authenticationWithCleanupP = authenticatedP.finally(() => {
+        timeout.cancel(Error('reason'));
+      });
+      // Prevent unhandled rejections
+      authenticationWithCleanupP.then(
+        () => {},
+        () => {},
+      );
+      authenticationEntry.authenticatedP = authenticationWithCleanupP;
       this.authenticationMap.set(nodeIdString, authenticationEntry);
     }
     const existingAuthenticate =
@@ -1349,10 +1395,14 @@ class NodeManager {
       throw new nodesErrors.ErrorNodeConnectionManagerConnectionNotFound();
     }
     try {
+      this.logger.warn('waiting');
       return await connectionsEntry.authenticatedP;
     } catch (e) {
+      this.logger.warn('failed waiting');
       Error.captureStackTrace(e);
       throw e;
+    } finally {
+      this.logger.warn('done');
     }
   }
 
@@ -1370,10 +1420,13 @@ class NodeManager {
       this.logger.warn('skipped');
       return;
     }
-    const rejectAuthentication = connectionsEntry.authenticatedRejectP;
+    const authenticatedRejectP = connectionsEntry.authenticatedRejectP;
     // Trigger shutdown of the connections
     await this.nodeConnectionManager.destroyConnection(nodeId, true);
-    let reason;
+    let reason: Error;
+    this.logger.warn(
+      `boi ${connectionsEntry.reasonForward}|${connectionsEntry.reasonReverse}`,
+    );
     if (
       connectionsEntry.reasonForward != null &&
       connectionsEntry.reasonReverse != null
@@ -1393,7 +1446,8 @@ class NodeManager {
       utils.never('No reason was provided');
     }
     this.logger.warn(`Authentication failed with: ${reason.toString()}`);
-    rejectAuthentication(
+    // Removing authentication entry
+    authenticatedRejectP(
       new nodesErrors.ErrorNodeManagerAuthenticationFailed(undefined, {
         cause: reason,
       }),

src/nodes/errors.ts

@@ -48,6 +48,16 @@ class ErrorNodeManagerAuthenticationFailed<T> extends ErrorNodeManager<T> {
   exitCode = sysexits.NOPERM;
 }
 
+class ErrorNodeManagerForwardAuthenticationFailed<T> extends ErrorNodes<T> {
+  static description = 'Failed to complete forward authentication';
+  exitCode = sysexits.USAGE;
+}
+
+class ErrorNodeManagerAuthenticatonTimedOut<T> extends ErrorNodes<T> {
+  static description = 'Failed to complete authentication before timing out';
+  exitCode = sysexits.USAGE;
+}
+
 class ErrorNodeGraph<T> extends ErrorNodes<T> {}
 
 class ErrorNodeGraphRunning<T> extends ErrorNodeGraph<T> {
@@ -245,6 +255,8 @@ export {
   ErrorNodeManagerSyncNodeGraphFailed,
   ErrorNodeManagerAuthenticationCallbackNotProvided,
   ErrorNodeManagerAuthenticationFailed,
+  ErrorNodeManagerForwardAuthenticationFailed,
+  ErrorNodeManagerAuthenticatonTimedOut,
   ErrorNodeGraph,
   ErrorNodeGraphRunning,
   ErrorNodeGraphNotRunning,

tests/nodes/NodeManager.test.ts

@@ -2295,11 +2295,7 @@ describe(`${NodeManager.name}`, () => {
       );
       await expect(reverseAuthenticateP).toResolve();
     });
-    // FIXME: the forward trigger fails so the reverse never starts. We need a timeout here.
-    // TODO: Forward authenticate callback throwing and error will not be a fully supported error path but
-    //  still needs to be handled.
-    // TODO: We need authentication triggered and cleaned up by connection life cycle.
-    // TODO: we need to garbage collect authentication state
+    // TODO: use the connection destroyed event to clean up auth state.
     test('forward authenticate fails on local', async () => {
       nodeManagerLocal.setAuthenticateNetworkForwardCallback(async () => {
         throw Error('Failure to generate forward authentication message');
@@ -2407,24 +2403,29 @@ describe(`${NodeManager.name}`, () => {
         nodeConnectionManagerPeer.port,
       );
 
+      logger.warn('step1');
       const authenticationAttemptP = nodeManagerLocal.withConnF(
         nodeIdPeer,
         async () => {
           // Do nothing
         },
       );
+      logger.warn('step2');
       await expect(authenticationAttemptP).rejects.toThrow(
         nodesErrors.ErrorNodeManagerAuthenticationFailed,
       );
+      logger.warn('step3');
       const forwardAuthenticateP = nodeManagerLocal.withConnF(
         nodeIdPeer,
         async () => {
           // Do nothing
         },
       );
+      logger.warn('step4');
       await expect(forwardAuthenticateP).rejects.toThrow(
         nodesErrors.ErrorNodeManagerAuthenticationFailed,
       );
+      logger.warn('step5');
       const reverseAuthenticateP = nodeManagerPeer.withConnF(
         nodeIdLocal,
         async () => {
@@ -2434,10 +2435,67 @@ describe(`${NodeManager.name}`, () => {
       await expect(reverseAuthenticateP).rejects.toThrow(
         nodesErrors.ErrorNodeManagerAuthenticationFailed,
       );
+      logger.warn('DONE');
     });
     test.todo('reverse authenticate fails on local');
     test.todo('reverse authenticate fails on peer');
     test.todo('non whitelisted RPC calls are prevented');
+    test.todo('connections that do not initiate authentication are handled');
+    test('can fail authentication with timeout', async () => {
+      const { p: waitP } = utils.promise();
+      nodeManagerLocal.setAuthenticateNetworkForwardCallback(async () => {
+        const message: NodesAuthenticateConnectionMessageBasicPublic = {
+          type: 'NodesAuthenticateConnectionMessageBasicPublic',
+          networkId: 'hello',
+        };
+        return message;
+      });
+      nodeManagerPeer.setAuthenticateNetworkForwardCallback(async () => {
+        const message: NodesAuthenticateConnectionMessageBasicPublic = {
+          type: 'NodesAuthenticateConnectionMessageBasicPublic',
+          networkId: 'hello',
+        };
+        return message;
+      });
+      nodeManagerLocal.setAuthenticateNetworkReverseCallback(
+        async (message) => {
+          if (
+            message.type !== 'NodesAuthenticateConnectionMessageBasicPublic'
+          ) {
+            throw Error('must be basic message');
+          }
+          if (message.networkId !== 'hello') {
+            throw Error('network must be "hello"');
+          }
+        },
+      );
+      nodeManagerPeer.setAuthenticateNetworkReverseCallback(async (message) => {
+        await waitP;
+        if (message.type !== 'NodesAuthenticateConnectionMessageBasicPublic') {
+          throw Error('must be basic message');
+        }
+        if (message.networkId !== 'hello') {
+          throw Error('network must be "hello"');
+        }
+      });
+
+      // Creating first connection to 0;
+      await nodeConnectionManagerLocal.createConnection(
+        [nodeIdPeer],
+        localHost,
+        nodeConnectionManagerPeer.port,
+      );
+
+      const forwardAuthenticateP = nodeManagerLocal.withConnF(
+        nodeIdPeer,
+        async () => {
+          // Do nothing
+        },
+      );
+      await expect(forwardAuthenticateP).rejects.toThrow(
+        nodesErrors.ErrorNodeManagerAuthenticationFailed,
+      );
+    });
     test.todo('');
   });
 });