nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.58k stars 1.47k forks source link

AsyncHttpClient not work with LibreSSL #20144

Closed jackyzy823 closed 1 year ago

jackyzy823 commented 2 years ago

AsyncHttpClient not work with LibreSSL

Example

Just the official example of httpclient with https url

import std/[asyncdispatch, httpclient]

proc asyncProc(): Future[string] {.async.} =
  var client = newAsyncHttpClient()
  return await client.getContent("https://example.com")

echo waitFor asyncProc()

Compile In OpenBSD 7.1 (which uses LibreSSL 3.5.2) nim c -d:ssl a.nim

Current Output

Error: unhandled exception: No error reported. Async traceback: /src/a.nim(9) a /nim/lib/pure/asyncdispatch.nim(1961) waitFor /nim/lib/pure/asyncdispatch.nim(1653) poll /nim/lib/pure/asyncdispatch.nim(1394) runOnce /nim/lib/pure/asyncdispatch.nim(234) processPendingCallbacks /nim/lib/pure/asyncmacro.nim(28) sendNimAsyncContinue /nim/lib/pure/asyncnet.nim(271) sendIter /nim/lib/pure/asyncnet.nim(221) getSslError /nim/lib/pure/net.nim(540) raiseSSLError

Expected Output

The content of URL

Possible Solution

I do not know whether it is a bug of nim or libressl.

So I have done some tests. Hope that will helps.

  1. The sync HttpClient work normally under Libressl 3.5.2 and other older LibreSSL( only test some version from 3.1.0).
  2. In Linux (docker image nimlang/nim) Compile Libressl 3.5.2 and running with preload LibreSSL via LD_LIBRARY_PATH (ie LD_LIBRARY_PATH=/path/libressl/lib nim_executable_file ) will also raise error.
  3. The latest working LibreSSL version is 3.2.0
  4. In LibreSSL 3.2.1 , default one not work and then i edit ssl/ssl_locl.h and comment out #define LIBRESSL_HAS_TLS1_3_CLIENT and recompile and it works again.

So i think there're something wrong with non-blocking mode and TLS 1.3.

So I think either LibreSSL's TLS1.3 implementaion under non-blocking mode has some bugs or nim's asyncnet misusing some LibreSSL's method.

It is too hard for me to find out the real reason of the bug. So it would be appreciated that anyone could help.

Additional Information

The sync HttpClient work normally.

import std/httpclient
var client = newHttpClient()
echo client.getContent("https://www.example.com")

$ nim -v
Nim Compiler Version 1.6.6
jackyzy823 commented 2 years ago

Some digging details:

LibreSSL reutrns return tls13_legacy_return_code(ssl, TLS13_IO_WANT_POLLOUT); https://github.com/libressl-portable/openbsd/blob/55f977dcaef2ca875e62631113886470bfc4b7dd/src/lib/libssl/tls13_legacy.c#L270 when finishing handshake in SSL_Write and then (if work correctly) asking Nim to send data again via error code SSL_ERROR_WANT_WRITE.

tls13_legacy_return_code set wbio 's flag to retry | write , https://github.com/libressl-portable/openbsd/blob/55f977dcaef2ca875e62631113886470bfc4b7dd/src/lib/libssl/tls13_legacy.c#L194

when nim do SSL_get_error , LibreSSL will return SSL_ERROR_WANT_WRITE if wbio's flag contains retry | write

However bioRead in sendPendingSslData will reset wbio's flag , will cause SSL_get_error return SSL_ERROR_SYSCALL

Then the bug happens.

However i do not know how to fix this :disappointed:

jackyzy823 commented 2 years ago

Well a bit ugly , Just save error code before call sendPendingSslData

template sslLoop(socket: AsyncSocket, flags: set[SocketFlag],
                 op: untyped) =
  var opResult {.inject.} = -1.cint
  while opResult < 0:
    ErrClearError()
    # Call the desired operation.
    opResult = op
    var err = SSL_ERROR_NONE
    if opResult < 0:
      err = getSslError(socket, opResult.cint)
    ## let bio = SSL_get_wbio(socket.sslHandle)
    ## echo "Bio in asyncnet", BIO_test_flags(bio, -1)

    # Send any remaining pending SSL data.
    await sendPendingSslData(socket, flags)
    ## echo "Bio in asyncnet after sendPendingSslData", BIO_test_flags(bio, -1)

    # If the operation failed, try to see if SSL has some data to read
    # or write.
    if opResult < 0:
      #let err = getSslError(socket, opResult.cint)
      let fut = appeaseSsl(socket, flags, err.cint)
      yield fut
      if not fut.read():
        # Socket disconnected.
        if SocketFlag.SafeDisconn in flags:
          opResult = 0.cint
          break
        else:
          raiseSSLError("Socket has been disconnected")
ringabout commented 2 years ago

Nice! Thanks for your efforts on this issue!

4a6f656c commented 2 years ago

For what it is worth, the SSL_ERROR_WANT_WRITE occurs since LibreSSL signals this upon completion of the handshake, in the case where a call to SSL_write() invokes the handshake (see https://github.com/libressl-portable/openbsd/blob/master/src/lib/libssl/tls13_legacy.c#L270). Furthermore, the proposed fix is correct - in both LibreSSL and OpenSSL, the value returned by SSL_get_error() is no longer reliable as soon as anything is done with the underlying BIO - reading or writing to the BIO will change its retry status and hence the value returned from SSL_get_error() will change as a result.

landryb commented 2 years ago

@jackyzy823 fwiw i've rebuilt nim 1.6.6 port on OpenBSD 7.2 with the below patch which seems to be more or less what you've put in your comment above:

Index: lib/pure/asyncnet.nim
--- lib/pure/asyncnet.nim.orig
+++ lib/pure/asyncnet.nim
@@ -261,6 +261,9 @@ when defineSsl:
       ErrClearError()
       # Call the desired operation.
       opResult = op
+      var err = SSL_ERROR_NONE
+      if opResult < 0:
+        err = getSslError(socket, opResult.cint)

       # Send any remaining pending SSL data.
       await sendPendingSslData(socket, flags)
@@ -268,7 +271,7 @@ when defineSsl:
       # If the operation failed, try to see if SSL has some data to read
       # or write.
       if opResult < 0:
-        let err = getSslError(socket, opResult.cint)
+        #let err = getSslError(socket, opResult.cint)
         let fut = appeaseSsl(socket, flags, err.cint)
         yield fut
         if not fut.read():

but it doesnt seem to fix the original issue here, and the basic testcase still fails. Do you have more details on what you've done to make it work ?

jackyzy823 commented 2 years ago

First i haven't done any unit test on this and so i don't know if this patch will break testcases. And i tested the patch in OpenBSD 7.2 again , and it works too. So i have no idea why it not work for you.

Env: OpenBSD 7.2 (pure install from install72.iso) LibreSSL 3.6.0 ( from openssl version) Nim 1.6.6p1 (pkg_add nim)

modified file /usr/local/lib/nim/pure/asyncnet.nim as what you did

tuftedocelot commented 1 year ago

FWIW, the patch works fine with OpenBSD-current which ships with LibreSSL 3.7.1 (as of today). This was tested with both the sample a.nim from https://github.com/nim-lang/Nim/issues/20144#issue-1327161283 as well as building/running nitter

tuftedocelot commented 1 year ago

Hello @jackyzy823, is there anything needed to push this through? This patch continues to work with latest nitter and LibreSSL 3.8.0 on OpenBSD-current

Araq commented 1 year ago

@ringabout please turn this patch into a PR.

ringabout commented 1 year ago

Sure thing