Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-28469

Improve SSLManager error handling

    • Fully Compatible
    • Dev Tools 2019-06-17

      These all relate to the code in and around SSLManager::_handleSSLError.

      • Empty string passed for the hostname/IP in the thrown SocketException:
            throw SocketException(SocketException::CONNECT_ERROR, "");
        

        This leads to strange log messages "socket exception [CONNECT_ERROR] for" (with no hostname or IP address after the "for"), which at first blush appear to have been abnormally truncated, eg:

        2017-03-17T15:32:38.301-0400 F STORAGE [initandlisten] Aborting due to exception in WT_ENCRYPTOR::customize: Location9001: socket exception [CONNECT_ERROR] for
        

        In fact _handleSSLError is always called within the context of an SSLConnection object. This should be passed into _handleSSLError so that the server name can be passed to the SocketException, ie. via remoteString() (same as is done in SSLManager::_flushNetworkBIO() and throughout sock.cpp).

        Unable to find source-code formatter for language: diff. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml
        -    throw SocketException(SocketException::CONNECT_ERROR, "");
        +    throw SocketException(SocketException::CONNECT_ERROR, sslConn->socket->remoteString());
        
      • CONNECT_ERROR is always thrown, even though _handleSSLError() is called from contexts other than connection establishment, such as SSL_read() and SSL_write(). In these cases RECV_ERROR and SEND_ERROR would be more appropriate. Thus _handleSSLError() should take an additional parameter which is the type of SocketException that should be thrown.
      • In the case of SSL_ERROR_ZERO_RETURN, a message is logged at LOG(3). This case indicates that the SSL stream has been (unexpectedly) closed — whether explicitly in the SSL protocol by the remote, or for some abnormal reason. It is important that this fact be logged so that users can be aware of it. Thus, the LOG(3) should be changed to error() (like the other cases).

        The code comment in this case of the switch suggests that it may be possible to re-establish a new SSL stream on the underlying transport, and to not throw a SocketException. If this later turns out to be possible and desirable, then the error() here could be updated accordingly (ie. changed back to LOG(3) or similar) at that time.

            Assignee:
            alya.berciu@mongodb.com Alya Berciu
            Reporter:
            kevin.pulo@mongodb.com Kevin Pulo
            Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated:
              Resolved: