From 40afe79b0ceaf4dff88c2a77c6d98722b90d3b93 Mon Sep 17 00:00:00 2001
From: Alejandro Ruiz <4057165+aruiz14@users.noreply.github.com>
Date: Mon, 1 Jul 2024 19:32:15 +0200
Subject: [PATCH] fix: use independent deadline for sending error (#79)

* fix: use independent deadline for sending error
* tests: adapt unit tests to cover this case

The other side of the connection relies on this error message being
received in order to close the connection, indefinitely keeping it open
otherwise. Taking this into account, this should be considered a control
message, not part of the connection but the session instead.
However, since c.writeDeadline is being used (which is indeed controlled
via net.Conn interface), the upper crypto/tls.Conn is setting this
deadline to an invalid value in order to prevent further writes.
This causes the error messages to never be sent.
---
 connection.go         |  3 ++-
 session_serve_test.go | 10 ++++++++--
 types.go              |  2 ++
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/connection.go b/connection.go
index 4653797..73da5ab 100644
--- a/connection.go
+++ b/connection.go
@@ -124,7 +124,8 @@ func (c *connection) writeErr(err error) {
 	if err != nil {
 		msg := newErrorMessage(c.connID, err)
 		metrics.AddSMTotalTransmitErrorBytesOnWS(c.session.clientKey, float64(len(msg.Bytes())))
-		if _, err2 := c.session.writeMessage(c.writeDeadline, msg); err2 != nil {
+		deadline := time.Now().Add(SendErrorTimeout)
+		if _, err2 := c.session.writeMessage(deadline, msg); err2 != nil {
 			logrus.Warnf("[%d] encountered error %q while writing error %q to close remotedialer", c.connID, err2, err)
 		}
 	}
diff --git a/session_serve_test.go b/session_serve_test.go
index 019fece..83ce5e5 100644
--- a/session_serve_test.go
+++ b/session_serve_test.go
@@ -109,7 +109,10 @@ func TestSession_closeConnection(t *testing.T) {
 	s := setupDummySession(t, 0)
 	var msg *message
 	s.conn = &fakeWSConn{
-		writeMessageCallback: func(msgType int, _ time.Time, data []byte) (err error) {
+		writeMessageCallback: func(msgType int, deadline time.Time, data []byte) (err error) {
+			if !deadline.IsZero() && deadline.Before(time.Now()) {
+				return errors.New("deadline exceeded")
+			}
 			msg, err = newServerMessage(bytes.NewReader(data))
 			return
 		},
@@ -118,13 +121,16 @@ func TestSession_closeConnection(t *testing.T) {
 	conn := newConnection(connID, s, "test", "test")
 	s.addConnection(connID, conn)
 
+	// Ensure Error message is sent regardless of the WriteDeadline value, see https://github.com/rancher/remotedialer/pull/79
+	_ = conn.SetWriteDeadline(time.Now())
+
 	expectedErr := errors.New("connection closed")
 	s.closeConnection(connID, expectedErr)
 
 	if s.getConnection(connID) != nil {
 		t.Errorf("connection was not closed correctly")
 	}
-	if conn.err == nil {
+	if conn.err == nil || msg == nil {
 		t.Fatal("message not sent on closed connection")
 	} else if msg.messageType != Error {
 		t.Errorf("incorrect message type sent")
diff --git a/types.go b/types.go
index 4af8981..3825581 100644
--- a/types.go
+++ b/types.go
@@ -11,4 +11,6 @@ const (
 	SyncConnectionsTimeout = 60 * time.Second
 	MaxRead                = 8192
 	HandshakeTimeOut       = 10 * time.Second
+	// SendErrorTimeout sets the maximum duration for sending an error message to close a single connection
+	SendErrorTimeout = 5 * time.Second
 )