fix(TLS/ConnectionHandler): Improve handling of TLS session resumption without SNI by sending an unrecognized_name alert instead of immediately terminating the connection. This change adds a grace period for the client to retry the handshake with proper SNI and cleans up the connection if no valid response is received.

changelog.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## 2025-03-16 - 4.1.5 - fix(TLS/ConnectionHandler)
+Improve handling of TLS session resumption without SNI by sending an 'unrecognized_name' alert instead of immediately terminating the connection. This change adds a grace period for the client to retry the handshake with proper SNI and cleans up the connection if no valid response is received.
+
+- Send a TLS warning (unrecognized_name alert, code 112) when a ClientHello is received without SNI and session tickets are disallowed.
+- Utilize socket cork/uncork to ensure the alert is sent as a single packet.
+- Add a 5-second alert timeout and a subsequent 30-second grace period to allow clients to initiate a new handshake with SNI.
+- Clean up and terminate the connection if no valid SNI is provided after the grace period, logging appropriate termination reasons.
+
 ## 2025-03-15 - 4.1.4 - fix(ConnectionHandler)
 Refactor ConnectionHandler code formatting for improved readability and consistency in log messages and whitespace handling
 

ts/00_commitinfo_data.ts

@@ -3,6 +3,6 @@
  */
 export const commitinfo = {
   name: '@push.rocks/smartproxy',
-  version: '4.1.4',
+  version: '4.1.5',
   description: 'A powerful proxy package that effectively handles high traffic, with features such as SSL/TLS support, port proxying, WebSocket handling, dynamic routing with authentication options, and automatic ACME certificate management.'
 }

ts/classes.pp.connectionhandler.ts

@@ -560,9 +560,18 @@ export class ConnectionHandler {
             // Always block ClientHello without SNI when allowSessionTicket is false
             console.log(
               `[${connectionId}] No SNI detected in ClientHello and allowSessionTicket=false. ` +
-                `Terminating connection to force new TLS handshake with SNI.`
+                `Sending unrecognized_name alert to encourage client to retry with SNI.`
             );
 
+            // Set the termination reason first to avoid races
+            if (record.incomingTerminationReason === null) {
+              record.incomingTerminationReason = 'session_ticket_blocked_no_sni';
+              this.connectionManager.incrementTerminationStat(
+                'incoming',
+                'session_ticket_blocked_no_sni'
+              );
+            }
+
             // Send a proper TLS alert before ending the connection
             // Using "unrecognized_name" (112) alert which is a warning level alert (1)
             // that encourages clients to retry with proper SNI
@@ -577,38 +586,95 @@ export class ConnectionHandler {
             ]);
 
             try {
-              socket.write(alertData, () => {
+              // Make sure the alert is sent as a single packet
-                // Only close the socket after we're sure the alert was sent
+              socket.cork();
-                // Give the alert time to be processed by the client
+              socket.write(alertData);
-                setTimeout(() => {
+              socket.uncork();
+              
+              // Set a longer timeout to allow the client to properly handle the alert
+              // The client might respond by closing the connection or initiating a new handshake
+              const alertTimeout = setTimeout(() => {
+                if (!socket.destroyed) {
+                  console.log(`[${connectionId}] Client didn't respond to TLS alert, closing connection gracefully.`);
+                  
+                  // Gracefully end the connection
+                  socket.end();
+                  
+                  // Only destroy after a delay if it's still hanging
+                  setTimeout(() => {
+                    if (!socket.destroyed) {
+                      console.log(`[${connectionId}] Forcibly closing connection that didn't terminate properly.`);
+                      socket.destroy();
+                    }
+                    this.connectionManager.cleanupConnection(record, 'session_ticket_blocked_no_sni');
+                  }, 2000);
+                }
+              }, 5000); // Give the client 5 seconds to respond to our alert
+              
+              // Don't let this timeout keep the process alive
+              if (alertTimeout.unref) {
+                alertTimeout.unref();
+              }
+              
+              // Handle a proper close from the client
+              socket.once('close', () => {
+                clearTimeout(alertTimeout);
+                console.log(`[${connectionId}] Client closed connection after receiving TLS alert.`);
+                this.connectionManager.cleanupConnection(record, 'session_ticket_blocked_no_sni_client_closed');
+              });
+              
+              // Also handle if the client sends more data (possibly a new handshake with SNI)
+              socket.once('data', (newChunk) => {
+                clearTimeout(alertTimeout);
+                console.log(`[${connectionId}] Client sent new data after TLS alert, checking for SNI...`);
+                
+                // This would normally be handled by our renegotiation handler,
+                // but since we're in a special case, we'll check for SNI again
+                if (this.tlsManager.isTlsHandshake(newChunk) && this.tlsManager.isClientHello(newChunk)) {
+                  const newServerName = this.tlsManager.extractSNI(newChunk, connInfo);
+                  
+                  if (newServerName) {
+                    console.log(`[${connectionId}] Client provided SNI in new handshake: ${newServerName}`);
+                    
+                    // Update the record
+                    record.incomingTerminationReason = null;
+                    
+                    // Remove termination stats increment
+                    // Note: This is a little hacky as we don't have a proper way to decrement stats
+                    
+                    // Process the new handshake with SNI
+                    record.lockedDomain = newServerName;
+                    setupConnection(newServerName, newChunk);
+                    return;
+                  } else {
+                    console.log(`[${connectionId}] Client sent new handshake but still without SNI, closing connection.`);
+                    socket.end();
+                    setTimeout(() => {
+                      if (!socket.destroyed) {
+                        socket.destroy();
+                      }
+                      this.connectionManager.cleanupConnection(record, 'session_ticket_blocked_no_sni_retry_failed');
+                    }, 500);
+                  }
+                } else {
+                  console.log(`[${connectionId}] Client sent non-handshake data after TLS alert, closing connection.`);
                   socket.end();
-
-                  // Ensure complete cleanup happens a bit later
                   setTimeout(() => {
                     if (!socket.destroyed) {
                       socket.destroy();
                     }
-                    this.connectionManager.cleanupConnection(
+                    this.connectionManager.cleanupConnection(record, 'session_ticket_blocked_invalid_response');
-                      record,
+                  }, 500);
-                      'session_ticket_blocked_no_sni'
+                }
-                    );
-                  }, 100);
-                }, 100);
               });
+              
             } catch (err) {
               // If we can't send the alert, fall back to immediate termination
+              console.log(`[${connectionId}] Error sending TLS alert: ${err.message}`);
               socket.end();
               this.connectionManager.cleanupConnection(record, 'session_ticket_blocked_no_sni');
             }
 
-            if (record.incomingTerminationReason === null) {
-              record.incomingTerminationReason = 'session_ticket_blocked_no_sni';
-              this.connectionManager.incrementTerminationStat(
-                'incoming',
-                'session_ticket_blocked_no_sni'
-              );
-            }
-
             return;
           }
         }
@@ -1056,4 +1122,4 @@ export class ConnectionHandler {
       }
     });
   }
-}
+}