Tls_lwt: delay error from writing to peer while reading. by hannesm · Pull Request #388 · mirleft/ocaml-tls

hannesm · 2019-01-04T20:10:21Z

previously, if handle_tls returned a response to the peer, write was called,
which may error out (recording it's error in the state and Lwt.fail). the
received data was never returned to the caller.

now, similar to the semantics in tls_mirage, the error from write is only
recorded in the state, and returned on the next function call (write/..).

should fix #347, alternative to #371, #387 -- could you please test this @edwintorok @anmonteiro @rymdhund //cc @pqwy

anmonteiro · 2019-01-05T00:31:09Z

@hannesm thanks for putting this together. I've tested this in my repro and it fixes the issue I was seeing.

rymdhund · 2019-01-05T18:01:22Z

With this patch i'm still getting

Fatal error: exception Unix.Unix_error(Unix.EPIPE, "Tls_lwt.write", "")                                                          
Raised at file "src/core/lwt.ml", line 2987, characters 20-29
Called from file "src/unix/lwt_main.ml", line 26, characters 8-18
Called from file "lwt/examples/http_client.ml", line 26, characters 39-85

hannesm · 2019-01-05T19:13:14Z

@rymdhund hmm, strange. I tested your example server and the provided lwt/examples/http_client.ml -- and I see the following (stripped) output:

(record-in
 (((content_type ALERT) (version (Supported TLS_1_2)))
  "\000\000\000\000\000\000\000\002A\194\254\133\"\248\026\022T\150\007\167\177\172\176t\228L"))

(alert-in (WARNING CLOSE_NOTIFY))

(record-out
 (ALERT
  "\000\000\000\000\000\000\000\002h\232!y=\163\154\204R\2324\147\145\014}:\0288"))

(eof-out ())

HTTP/1.1 200 OK
content-length: 11

hello world++ done.

note that I'm using cohttp* pinned to git+https://github.com/mirage/ocaml-cohttp.git, conduit* pinned to git+https://github.com/avsm/ocaml-conduit.git#ipaddr-3 (see mirage/ocaml-conduit#284)

I also tested with a fresh switch and only released packages (not even using this PR), which resulted in no exception, but the same behaviour as above.

$ opam list
# Packages matching: installed
# Name                  # Installed # Synopsis
asn1-combinators        0.2.0       Embed typed ASN.1 grammars in OCaml
astring                 0.8.3       Alternative String module for OCaml
base                    v0.11.1     Full standard library replacement for OCaml
base-bigarray           base
base-bytes              base        Bytes library distributed with the OCaml compiler
base-threads            base
base-unix               base
base64                  2.3.0       Base64 encoding for OCaml
cmdliner                1.0.3       Declarative definition of command line interfaces for OCaml
cohttp                  1.2.0       An OCaml library for HTTP clients and servers
cohttp-lwt              1.2.0       An OCaml library for HTTP clients and servers
cohttp-lwt-unix         1.2.0       An OCaml library for HTTP clients and servers
conduit                 1.3.0       An OCaml network connection establishment library
conduit-lwt             1.3.0       An OCaml network connection establishment library
conduit-lwt-unix        1.3.0       An OCaml network connection establishment library
conf-gmp                1           Virtual package relying on a GMP lib system installation
conf-m4                 1           Virtual package relying on m4
conf-perl               1           Virtual package relying on perl
cppo                    1.6.5       Equivalent of the C preprocessor for OCaml programs
cppo_ocamlbuild         1.6.0       ocamlbuild support for cppo, OCaml-friendly source preprocessor
cpuid                   0.1.1       Detect CPU features
cstruct                 3.2.1       Access C-like structures directly from OCaml
cstruct-lwt             3.2.1       Access C-like structures directly from OCaml
dune                    1.6.2       Fast, portable and opinionated build system
fieldslib               v0.11.0     Syntax extension to define first class values representing record fields, to get and set record fields, iterate and fold over all fields of a record and create new record values
fmt                     0.8.5       OCaml Format pretty-printer combinators
ipaddr                  2.9.0       A library for manipulation of IP (and MAC) address representations
jbuilder                transition  This is a transition package, jbuilder is now named dune. Use the dune
jsonm                   1.0.1       Non-blocking streaming JSON codec for OCaml
logs                    0.6.2       Logging infrastructure for OCaml
lwt                     4.1.0       Promises, concurrency, and parallelized I/O
magic-mime              1.1.1       Map filenames to common MIME types
mirage-no-solo5         1           Virtual package conflicting with mirage-solo5
mirage-no-xen           1           Virtual package conflicting with mirage-xen
nocrypto                0.5.4-1     Simpler crypto
num                     1.1         The legacy Num library for arbitrary-precision integer and rational arithmetic
ocaml                   4.07.1      The OCaml compiler (virtual package)
ocaml-base-compiler     4.07.1      Official release 4.07.1
ocaml-compiler-libs     v0.11.0     OCaml compiler libraries repackaged
ocaml-config            1           OCaml Switch Configuration
ocaml-migrate-parsetree 1.1.0
ocamlbuild              0.12.0      OCamlbuild is a build system with builtin rules to easily build most OCaml projects.
ocamlfind               1.8.0       A library manager for OCaml
ocb-stubblr             0.1.1-1     OCamlbuild plugin for C stubs
parsexp                 v0.11.0     S-expression parsing library
ppx_cstruct             3.2.1       Access C-like structures directly from OCaml
ppx_derivers            1.0         Shared [@@deriving] plugin registry
ppx_deriving            4.2.1       Type-driven code generation for OCaml >=4.02
ppx_fields_conv         v0.11.0     Generation of accessor and iteration functions for ocaml records
ppx_sexp_conv           v0.11.2     Generation of S-expression conversion functions from type definitions
ppx_tools               5.1+4.06.0  Tools for authors of ppx rewriters and other syntactic tools
ppx_tools_versioned     5.2.1       A variant of ppx_tools based on ocaml-migrate-parsetree
ppxlib                  0.4.0       Base library and tools for ppx rewriters
ptime                   0.8.4       POSIX time for OCaml
re                      1.8.0       RE is a regular expression library for OCaml
result                  1.3         Compatibility Result module
seq                     base        Compatibility package for OCaml's standard iterator type starting from 4.07.
sexplib                 v0.11.0     Library for serializing OCaml values to and from S-expressions
sexplib0                v0.11.0     Library containing the definition of S-expressions and some base converters
stdio                   v0.11.0     Standard IO library for OCaml
stringext               1.5.0       Extra string functions for OCaml
tls                     0.9.2       Transport Layer Security purely in OCaml
topkg                   1.0.0       The transitory OCaml software packager
uchar                   0.0.2       Compatibility library for OCaml's Uchar module
uri                     2.1.0       An RFC3986 URI/URL parsing library
uutf                    1.0.1       Non-blocking streaming Unicode codec for OCaml
x509                    0.6.2       Public Key Infrastructure purely in OCaml
zarith                  1.7         Implements arithmetic and logical operations over arbitrary-precision integers

any chance an opam update && opam upgrade fixes your issue?

rymdhund · 2019-01-06T11:10:34Z

@hannesm that is strange! I'm getting the problem with exactly the same opam list as you. Perhaps your pinned conduit or cohttp changes the behaviour of the server somehow?

Here's a dockerfile that replicates my problem. Building and running it will give you the Fatal error: exception Unix.Unix_error(Unix.EPIPE, "Tls_lwt.write", "")
https://github.com/rymdhund/ocaml-tls-epipe-poc

hannesm · 2019-01-06T13:05:35Z

@rymdhund thanks. the list of opam packages above does not use any pins, only released packages (I installed this into a different switch). I'm testing on FreeBSD system (no docker here), and will try to reproduce on a GNU/Linux system later today.

Do you have the same conduit and cohttp versions installed as I have (in the above opam output)? Which ocaml version do you use?

hannesm · 2019-01-06T18:20:03Z

@rymdhund could reproduce on a virtualized Debian GNU/Linux system. I committed d1eda58 which avoids overwriting the `Eof state with an `Error from write, and thus Lwt_io read succeeds. could you verify that this fixes the issue for you as well!?

rymdhund · 2019-01-06T20:06:36Z

@hannesm great! That fixed my issue :)

previously, if handle_tls returned a response to the peer, write was called, which may error out (recording it's error in the state and Lwt.fail). the received data was never returned to the caller. now, similar to the semantics in tls_mirage, the error from write is only recorded in the state, and returned on the next function call (write/..).

this ensures proper Lwt_io interoperability, esp. if an Eof has been received, which in the TLS world is replied to with a close_notify alert -- this write can certainly fail. earlier, this resulted in an `Error state, now the `Eof state is retained and Lwt_io can read all application data.

…or case appear

…irleft#388

Since tls 0.9.3, the specific issue (error after eof) has been fixed. See mirleft/ocaml-tls#388 for the applied fix.

hannesm mentioned this pull request Jan 5, 2019

Don't try writing to the remote if we got EOF #387

Closed

hannesm added 3 commits January 7, 2019 21:07

changes for 0.9.3

c36c66f

hannesm force-pushed the lwt-resp-semantics branch from d1eda58 to c36c66f Compare January 7, 2019 20:08

hannesm merged commit c36c66f into mirleft:master Jan 7, 2019

hannesm deleted the lwt-resp-semantics branch January 9, 2019 08:59

hannesm referenced this pull request in dinosaure/ocaml-tls Jan 14, 2019

Delete some raise, save exception on the state and do nothing if `Err…

e72a996

…or case appear

dinosaure added a commit to dinosaure/ocaml-tls that referenced this pull request Jan 18, 2019

Add a comment to explain diff between async and lwt when we consider m…

1c11abf

…irleft#388

hannesm mentioned this pull request Feb 4, 2019

since tls 0.9.3, the specific issue (error after eof) has been fixed leostera/httpkit#14

Merged

leostera pushed a commit to leostera/httpkit that referenced this pull request Feb 4, 2019

[docs] Remove TLS pinning from README

af5700e

Since tls 0.9.3, the specific issue (error after eof) has been fixed. See mirleft/ocaml-tls#388 for the applied fix.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tls_lwt: delay error from writing to peer while reading.#388

Tls_lwt: delay error from writing to peer while reading.#388
hannesm merged 3 commits into
mirleft:masterfrom
hannesm:lwt-resp-semantics

hannesm commented Jan 4, 2019

Uh oh!

anmonteiro commented Jan 5, 2019

Uh oh!

rymdhund commented Jan 5, 2019

Uh oh!

hannesm commented Jan 5, 2019 •

edited

Loading

Uh oh!

rymdhund commented Jan 6, 2019

Uh oh!

hannesm commented Jan 6, 2019 •

edited

Loading

Uh oh!

hannesm commented Jan 6, 2019

Uh oh!

rymdhund commented Jan 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

hannesm commented Jan 4, 2019

Uh oh!

anmonteiro commented Jan 5, 2019

Uh oh!

rymdhund commented Jan 5, 2019

Uh oh!

hannesm commented Jan 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rymdhund commented Jan 6, 2019

Uh oh!

hannesm commented Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannesm commented Jan 6, 2019

Uh oh!

rymdhund commented Jan 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

hannesm commented Jan 5, 2019 •

edited

Loading

hannesm commented Jan 6, 2019 •

edited

Loading