Handle TLS servers that do not close connections gracefully.

Some TLS server implementations will simply close the socket rather than correctly closing the TLS connection. This causes problems when connection: close is specified with no content-length or chunked encoding and we are forced to read to EOF. It is hard to know if this is a real EOF or a network error. In cases where we can parse the content and (hopefully) ensure it is correct, allow the closed socket to serve as EOF. This is not ideal, but the change in 8e1807c means that currently working servers with this issue will stop working after 2.35 is installed, which seems too risky.
pgbackrest · Mar 2, 2022 · 59a5373 · 59a5373
1 parent f1bdf3e
commit 59a5373
Show file tree

Hide file tree

Showing 16 changed files with 183 additions and 48 deletions.
diff --git a/doc/xml/release.xml b/doc/xml/release.xml
@@ -108,6 +108,21 @@
                         <p>Retry on page validation failure during <cmd>backup</cmd>.</p>
                     </release-item>
 
+                    <release-item>
+                        <github-issue id="1638"/>
+                        <github-pull-request id="1677"/>
+
+                        <release-item-contributor-list>
+                            <release-item-contributor id="david.steele"/>
+                            <!-- Actually tester, but we don't have a tag for that yet -->
+                            <release-item-reviewer id="remi.vidier"/>
+                            <release-item-reviewer id="david.christensen"/>
+                            <release-item-reviewer id="stephen.frost"/>
+                        </release-item-contributor-list>
+
+                        <p>Handle <proper>TLS</proper> servers that do not close connections gracefully.</p>
+                    </release-item>
+
                     <release-item>
                         <github-pull-request id="1610"/>
 

diff --git a/src/command/server/ping.c b/src/command/server/ping.c
@@ -37,7 +37,7 @@ cmdServerPing(void)
         // Send ping
         ProtocolClient *const protocolClient = protocolClientNew(
             strNewFmt(PROTOCOL_SERVICE_REMOTE " socket protocol on '%s'", strZ(host)), PROTOCOL_SERVICE_REMOTE_STR,
-            ioSessionIoRead(tlsSession), ioSessionIoWrite(tlsSession));
+            ioSessionIoReadP(tlsSession), ioSessionIoWrite(tlsSession));
         protocolClientNoExit(protocolClient);
         protocolClientNoOp(protocolClient);
         protocolClientFree(protocolClient);

diff --git a/src/common/io/http/request.c b/src/common/io/http/request.c
@@ -29,6 +29,8 @@ STRING_EXTERN(HTTP_HEADER_CONTENT_MD5_STR,                          HTTP_HEADER_
 STRING_EXTERN(HTTP_HEADER_CONTENT_RANGE_STR,                        HTTP_HEADER_CONTENT_RANGE);
 STRING_EXTERN(HTTP_HEADER_CONTENT_TYPE_STR,                         HTTP_HEADER_CONTENT_TYPE);
 STRING_EXTERN(HTTP_HEADER_CONTENT_TYPE_APP_FORM_URL_STR,            HTTP_HEADER_CONTENT_TYPE_APP_FORM_URL);
+STRING_EXTERN(HTTP_HEADER_CONTENT_TYPE_JSON_STR,                    HTTP_HEADER_CONTENT_TYPE_JSON);
+STRING_EXTERN(HTTP_HEADER_CONTENT_TYPE_XML_STR,                     HTTP_HEADER_CONTENT_TYPE_XML);
 STRING_EXTERN(HTTP_HEADER_ETAG_STR,                                 HTTP_HEADER_ETAG);
 STRING_EXTERN(HTTP_HEADER_DATE_STR,                                 HTTP_HEADER_DATE);
 STRING_EXTERN(HTTP_HEADER_HOST_STR,                                 HTTP_HEADER_HOST);

diff --git a/src/common/io/http/request.h b/src/common/io/http/request.h
@@ -49,6 +49,10 @@ HTTP Constants
     STRING_DECLARE(HTTP_HEADER_CONTENT_TYPE_STR);
 #define HTTP_HEADER_CONTENT_TYPE_APP_FORM_URL                       "application/x-www-form-urlencoded"
     STRING_DECLARE(HTTP_HEADER_CONTENT_TYPE_APP_FORM_URL_STR);
+#define HTTP_HEADER_CONTENT_TYPE_JSON                               "application/json"
+    STRING_DECLARE(HTTP_HEADER_CONTENT_TYPE_JSON_STR);
+#define HTTP_HEADER_CONTENT_TYPE_XML                                "application/xml"
+    STRING_DECLARE(HTTP_HEADER_CONTENT_TYPE_XML_STR);
 #define HTTP_HEADER_CONTENT_RANGE_BYTES                             "bytes"
 #define HTTP_HEADER_DATE                                            "date"
     STRING_DECLARE(HTTP_HEADER_DATE_STR);

diff --git a/src/common/io/http/response.c b/src/common/io/http/response.c
@@ -76,6 +76,21 @@ httpResponseDone(HttpResponse *this)
 /***********************************************************************************************************************************
 Read content
 ***********************************************************************************************************************************/
+// Helper to determine if it is OK to accept unexpected EOF, e.g. the server closed the socket with properly closing the TLS
+// connection. The idea is that since these types of responses can be validated we should be able to detect a short read.
+static bool
+httpResponseReadIgnoreUnexpectedEof(const HttpResponse *const this)
+{
+    FUNCTION_LOG_BEGIN(logLevelTrace);
+        FUNCTION_LOG_PARAM(HTTP_RESPONSE, this);
+    FUNCTION_LOG_END();
+
+    const String *const contentType = httpHeaderGet(this->pub.header, HTTP_HEADER_CONTENT_TYPE_STR);
+
+    FUNCTION_LOG_RETURN(
+        BOOL, strEq(contentType, HTTP_HEADER_CONTENT_TYPE_XML_STR) || strEq(contentType, HTTP_HEADER_CONTENT_TYPE_JSON_STR));
+}
+
 static size_t
 httpResponseRead(THIS_VOID, Buffer *buffer, bool block)
 {
@@ -99,17 +114,20 @@ httpResponseRead(THIS_VOID, Buffer *buffer, bool block)
     {
         MEM_CONTEXT_TEMP_BEGIN()
         {
-            IoRead *rawRead = httpSessionIoRead(this->session);
-
             // If close was requested and no content specified then the server may send content up until the eof
             if (this->closeOnContentEof && !this->contentChunked && this->contentSize == 0)
             {
+                IoRead *const rawRead = httpSessionIoReadP(
+                    this->session, .ignoreUnexpectedEof = httpResponseReadIgnoreUnexpectedEof(this));
+
                 ioRead(rawRead, buffer);
                 this->contentEof = ioReadEof(rawRead);
             }
             // Else read using specified encoding or size
             else
             {
+                IoRead *const rawRead = httpSessionIoReadP(this->session);
+
                 do
                 {
                     // If chunked content and no content remaining
@@ -218,7 +236,7 @@ httpResponseNew(HttpSession *session, const String *verb, bool contentCache)
         MEM_CONTEXT_TEMP_BEGIN()
         {
             // Read status
-            String *status = ioReadLine(httpSessionIoRead(this->session));
+            String *status = ioReadLine(httpSessionIoReadP(this->session));
 
             // Check status ends with a CR and remove it to make error formatting easier and more accurate
             if (!strEndsWith(status, CR_STR))
@@ -260,7 +278,7 @@ httpResponseNew(HttpSession *session, const String *verb, bool contentCache)
             do
             {
                 // Read the next header
-                String *header = strTrim(ioReadLine(httpSessionIoRead(this->session)));
+                String *const header = strTrim(ioReadLine(httpSessionIoReadP(this->session)));
 
                 // If the header is empty then we have reached the end of the headers
                 if (strSize(header) == 0)

diff --git a/src/common/io/http/session.c b/src/common/io/http/session.c
@@ -63,15 +63,16 @@ httpSessionDone(HttpSession *this)
 
 /**********************************************************************************************************************************/
 IoRead *
-httpSessionIoRead(HttpSession *this)
+httpSessionIoRead(HttpSession *const this, const HttpSessionIoReadParam param)
 {
     FUNCTION_TEST_BEGIN();
         FUNCTION_TEST_PARAM(HTTP_SESSION, this);
+        FUNCTION_TEST_PARAM(BOOL, param.ignoreUnexpectedEof);
     FUNCTION_TEST_END();
 
     ASSERT(this != NULL);
 
-    FUNCTION_TEST_RETURN(ioSessionIoRead(this->ioSession));
+    FUNCTION_TEST_RETURN(ioSessionIoReadP(this->ioSession, .ignoreUnexpectedEof = param.ignoreUnexpectedEof));
 }
 
 /**********************************************************************************************************************************/

diff --git a/src/common/io/http/session.h b/src/common/io/http/session.h
@@ -39,7 +39,16 @@ void httpSessionDone(HttpSession *this);
 Getters/Setters
 ***********************************************************************************************************************************/
 // Read interface
-IoRead *httpSessionIoRead(HttpSession *this);
+typedef struct HttpSessionIoReadParam
+{
+    VAR_PARAM_HEADER;
+    bool ignoreUnexpectedEof;
+} HttpSessionIoReadParam;
+
+#define httpSessionIoReadP(this, ...)                                                                                                \
+    httpSessionIoRead(this, (HttpSessionIoReadParam){VAR_PARAM_INIT, __VA_ARGS__})
+
+IoRead *httpSessionIoRead(HttpSession *this, HttpSessionIoReadParam param);
 
 // Write interface
 IoWrite *httpSessionIoWrite(HttpSession *this);

diff --git a/src/common/io/session.h b/src/common/io/session.h
@@ -51,10 +51,19 @@ ioSessionAuthenticated(const IoSession *const this)
 int ioSessionFd(IoSession *this);
 
 // Read interface
+typedef struct IoSessionIoReadParam
+{
+    VAR_PARAM_HEADER;
+    bool ignoreUnexpectedEof;                                       // Allow unexpected EOF, e.g. TLS session improperly terminated
+} IoSessionIoReadParam;
+
+#define ioSessionIoReadP(this, ...)                                                                                                \
+    ioSessionIoRead(this, (IoSessionIoReadParam){VAR_PARAM_INIT, __VA_ARGS__})
+
 __attribute__((always_inline)) static inline IoRead *
-ioSessionIoRead(IoSession *const this)
+ioSessionIoRead(IoSession *const this, const IoSessionIoReadParam param)
 {
-    return THIS_PUB(IoSession)->interface->ioRead(THIS_PUB(IoSession)->driver);
+    return THIS_PUB(IoSession)->interface->ioRead(THIS_PUB(IoSession)->driver, param.ignoreUnexpectedEof);
 }
 
 // Write interface

diff --git a/src/common/io/session.intern.h b/src/common/io/session.intern.h
@@ -22,7 +22,7 @@ typedef struct IoSessionInterface
     int (*fd)(void *driver);
 
     // IoRead interface for the session
-    IoRead *(*ioRead)(void *driver);
+    IoRead *(*ioRead)(void *driver, bool ignoreUnexpectedEof);
 
     // IoWrite interface for the session
     IoWrite *(*ioWrite)(void *driver);

diff --git a/src/common/io/socket/session.c b/src/common/io/socket/session.c
@@ -106,12 +106,13 @@ sckSessionFd(THIS_VOID)
 
 /**********************************************************************************************************************************/
 static IoRead *
-sckSessionIoRead(THIS_VOID)
+sckSessionIoRead(THIS_VOID, const bool ignoreUnexpectedEof)
 {
     THIS(SocketSession);
 
     FUNCTION_TEST_BEGIN();
         FUNCTION_TEST_PARAM(SOCKET_SESSION, this);
+        (void)ignoreUnexpectedEof;                                  // Unused
     FUNCTION_TEST_END();
 
     ASSERT(this != NULL);

diff --git a/src/common/io/tls/session.c b/src/common/io/tls/session.c
@@ -25,6 +25,7 @@ typedef struct TlsSession
     SSL *session;                                                   // TLS session on the file descriptor
     TimeMSec timeout;                                               // Timeout for any i/o operation (read, write, etc.)
     bool shutdownOnClose;                                           // Shutdown the TLS connection when closing the session
+    bool ignoreUnexpectedEof;                                       // Ignore unexpected EOF when requested
 
     IoRead *read;                                                   // Read interface
     IoWrite *write;                                                 // Write interface
@@ -128,18 +129,30 @@ tlsSessionResultProcess(TlsSession *this, int errorTls, long unsigned int errorT
     {
         // The connection was closed
         case SSL_ERROR_ZERO_RETURN:
+        // A syscall failed (this usually indicates unexpected eof)
+        case SSL_ERROR_SYSCALL:
         {
-            if (!closeOk)
-                THROW(ProtocolError, "unexpected TLS eof");
+            // Error on SSL_ERROR_SYSCALL if unexpected EOF is not allowed
+            if (errorTls == SSL_ERROR_SYSCALL && !this->ignoreUnexpectedEof)
+            {
+                THROW_SYS_ERROR_CODE(errorSys, KernelError, "TLS syscall error");
+            }
+            // Else close the connection if we are in a state where it is allowed, e.g. not connecting
+            else
+            {
+                if (!closeOk)
+                    THROW(ProtocolError, "unexpected TLS eof");
+
+                this->shutdownOnClose = false;
+                tlsSessionClose(this);
+            }
 
-            this->shutdownOnClose = false;
-            tlsSessionClose(this);
             break;
         }
 
         // Try again after waiting for read ready
         case SSL_ERROR_WANT_READ:
-            ioReadReadyP(ioSessionIoRead(this->ioSession), .error = true);
+            ioReadReadyP(ioSessionIoReadP(this->ioSession), .error = true);
             result = 0;
             break;
 
@@ -149,10 +162,6 @@ tlsSessionResultProcess(TlsSession *this, int errorTls, long unsigned int errorT
             result = 0;
             break;
 
-        // A syscall failed (this usually indicates unexpected eof)
-        case SSL_ERROR_SYSCALL:
-            THROW_SYS_ERROR_CODE(errorSys, KernelError, "TLS syscall error");
-
         // Any other error that we cannot handle
         default:
         {
@@ -220,7 +229,7 @@ tlsSessionRead(THIS_VOID, Buffer *buffer, bool block)
     {
         // If no TLS data pending then check the io to reduce blocking
         if (!SSL_pending(this->session))
-            ioReadReadyP(ioSessionIoRead(this->ioSession), .error = true);
+            ioReadReadyP(ioSessionIoReadP(this->ioSession), .error = true);
 
         // Read and handle errors. The error queue must be cleared before this operation.
         ERR_clear_error();
@@ -292,16 +301,19 @@ tlsSessionEof(THIS_VOID)
 
 /**********************************************************************************************************************************/
 static IoRead *
-tlsSessionIoRead(THIS_VOID)
+tlsSessionIoRead(THIS_VOID, const bool ignoreUnexpectedEof)
 {
     THIS(TlsSession);
 
     FUNCTION_TEST_BEGIN();
         FUNCTION_TEST_PARAM(TLS_SESSION, this);
+        FUNCTION_TEST_PARAM(BOOL, ignoreUnexpectedEof);
     FUNCTION_TEST_END();
 
     ASSERT(this != NULL);
 
+    this->ignoreUnexpectedEof = ignoreUnexpectedEof;
+
     FUNCTION_TEST_RETURN(this->read);
 }
 

diff --git a/src/protocol/helper.c b/src/protocol/helper.c
@@ -373,7 +373,7 @@ protocolServer(IoServer *const tlsServer, IoSession *const socketSession)
         IoSession *const tlsSession = ioServerAccept(tlsServer, socketSession);
 
         result = protocolServerNew(
-            PROTOCOL_SERVICE_REMOTE_STR, PROTOCOL_SERVICE_REMOTE_STR, ioSessionIoRead(tlsSession),
+            PROTOCOL_SERVICE_REMOTE_STR, PROTOCOL_SERVICE_REMOTE_STR, ioSessionIoReadP(tlsSession),
             ioSessionIoWrite(tlsSession));
 
         // If session is authenticated
@@ -681,7 +681,7 @@ protocolRemoteExec(
                 .keyFile = cfgOptionIdxStr(isRepo ? cfgOptRepoHostKeyFile : cfgOptPgHostKeyFile, hostIdx));
             helper->ioSession = ioClientOpen(helper->ioClient);
 
-            read = ioSessionIoRead(helper->ioSession);
+            read = ioSessionIoReadP(helper->ioSession);
             write = ioSessionIoWrite(helper->ioSession);
 
             break;

diff --git a/test/src/common/harnessServer.c b/test/src/common/harnessServer.c
@@ -320,7 +320,7 @@ void hrnServerRun(IoRead *read, HrnServerProtocol protocol, HrnServerRunParam pa
 
                 TRY_BEGIN()
                 {
-                    ioRead(ioSessionIoRead(serverSession), buffer);
+                    ioRead(ioSessionIoReadP(serverSession), buffer);
                 }
                 CATCH(FileReadError)
                 {