Async support by dinosaure · Pull Request #1 · janestreet/ocaml-tls

dinosaure · 2018-08-29T13:48:02Z

No description provided.

This reverts commit 0b41d63.

avsm · 2018-10-10T16:09:38Z

+  | `AES_256_CBC_SHA256
+  | `AES_256_CCM
+  | `AES_256_GCM_SHA384
+  | `_3DES_EDE_CBC_SHA


I noticed this due to the odd type name, and then wondered if we shouldn't be disabling the 3DES ciphers by default due to their insecurity. See here for a discussion about this particular cipher combination and its effective entropy. If most clients in the wild support AES ciphers, then perhaps we can drop the older ones from async-tls.

…ackage on libraries)

seliopou

Ok I'm back on review. Sorry it's taken so long.

As a general comment, there are some functions in the tls_async.ml that raise when really they could just return a Deferred.Or_error.t. That would make "exception-raising" behavior of the functions more explicit and is a bit more idiomatic. More coming but I thought I'd point this out as I continue on with more specific comments.

seliopou · 2018-12-06T14:44:21Z

As a concrete example of a function that should return Deferred.Or_error.t, look at reneg. I imagine others could benefit from this change as well.

seliopou · 2018-12-06T14:46:51Z

Another piece of general feedback, there are a lot of places where assert false is being used, I believe, to just raise an error. See the TODO in reneg and the assert false in accept. These should all be replaced with informative error messages. If an assert false is used to indicate an impossible case, that's fine, but a comment as to why should also accompany it. I see that this approach was taken in epoch, which is great!

seliopou · 2018-12-06T14:56:57Z

+        cstruct_to_bigstring buf
+        @@ fun buf ~pos ~len ->
+        Unix.Syscall_result.Int.ok_or_unix_error_exn ~syscall_name:"read"
+        @@ Bigstring.read_assume_fd_is_nonblocking fd buf ~pos ~len


Stylistic, but this @@ style is a kindof unreadable.

… we have the same behavior

dinosaure · 2018-12-18T14:48:07Z

I think, all serious feedbacks was done with the second pass (with more explicit commits). At the end, stylistic issue can be fixed and _3DES_EDE_CBC_SHA can be removed. I removed all assert false (noticed with a TODO). Most of them are replaced by an exception. On the LWT back-end, it's exactly the same - but, instead to have our own exceptions, LWT back-end raises Invalid_argument.

dinosaure · 2018-12-18T14:48:58Z

And if all are happy with this impl. it could be a gift for Christmas?

upgrade to opam2

this fixes interoperability with (at least) android 4.4 devices

lib: do not require the list of ciphersuites sent by the client to be a proper set

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.

…ad raised

…or case appear

…irleft#388

cfcs · 2019-07-09T16:17:43Z

+                Fpath.pp path Error.pp err ;
+              [] )
+
+let authenticator meth =


Is there a reason that now not just called time so it can be applied with ~time instead of ~time:now ?

This reads the current time and does time conversion for all cases, including `No_authentication where it's not needed.
ping @dinosaure

Hmmhmm, for sure we can update time to be unit -> Ptime.t and call it then.

dinosaure added 7 commits July 19, 2018 17:49

First version of ocaml-tls on async

f64c242

Implement high level interface

1aecc04

Add tracing, loop on reader and writer and return Ok on the reader

6a5dadb

A multiple chain with single certificate is a single and clean code

c72ed19

Remove ppx_sexp_conv constraint

0b41d63

Use the async implementation of nocrypto-entropy

bc33433

Revert "Remove ppx_sexp_conv constraint"

411ab0f

This reverts commit 0b41d63.

avsm reviewed Oct 10, 2018

View reviewed changes

seliopou reviewed Oct 10, 2018

View reviewed changes

dinosaure added 3 commits October 29, 2018 16:16

Merge remote-tracking branch 'upstream/master' into async

37a3335

ocamlformat pass

a96e1af

First pass after @seliopou's review (most about deletion of Rresult p…

4f30d6e

…ackage on libraries)

seliopou reviewed Dec 6, 2018

View reviewed changes

Comment thread async/lib/tls_async.ml Outdated

seliopou reviewed Dec 6, 2018

View reviewed changes

Comment thread async/lib/tls_async.ml Outdated

seliopou reviewed Dec 6, 2018

View reviewed changes

Comment thread async/lib/tls_async.ml Outdated

seliopou reviewed Dec 6, 2018

View reviewed changes

seliopou requested changes Dec 6, 2018

View reviewed changes

Comment thread async/lib/tls_async.ml Outdated

dinosaure added 7 commits December 18, 2018 15:34

Remove useless function in x509_async.ml

8dd2a0f

Use Core.Or_error.t in x509_async.ml instead result type

125c1c2

Remove useless function in tls_async.ml

d125867

Add Bad_fd exception when syscall raise it

ffaec5e

Buffer per state instead global buffer in tls_async.ml

6927817

Add exception with documentation instead assert false - from lwt pov,…

1fc1503

… we have the same behavior

Update thales to use Core.Or_error.t instead result type

2d416ba

hannesm added 2 commits January 7, 2019 17:18

upgrade to opam2

c92820a

Merge pull request mirleft#389 from hannesm/opam2

997c413

upgrade to opam2

hannesm and others added 14 commits January 7, 2019 20:50

lib: do not require ciphersuites sent by the client to be a proper set.

b28b420

this fixes interoperability with (at least) android 4.4 devices

Merge pull request mirleft#390 from hannesm/no-require-cc-set

f705d6a

lib: do not require the list of ciphersuites sent by the client to be a proper set

changes for 0.9.3

c36c66f

update test scripts for openssl 1.1.1 (no more 3des and rc4)

fcd965e

Fix typo on Tls_can't_renegotiate

fa2ca74

Remove raise of Bad_fd - exception still exists but it returned inste…

50960f4

…ad raised

Fix typo on Tls_can't_renegociate

78e4b73

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

e72a996

…or case appear

Merge remote-tracking branch 'upstream/master' into async

5252bbf

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

1c11abf

…irleft#388

Delete all exceptions in tls_async.ml module

7fcbb7c

Update thales from the new interface of tls_async.ml

a4405c4

cfcs reviewed Jul 9, 2019

View reviewed changes

dinosaure mentioned this pull request Nov 16, 2020

conduit-tls must be thread-safe mirage/ocaml-conduit#336

Closed

torinnd mentioned this pull request May 17, 2021

Async implementation, including examples mirleft/ocaml-tls#432

Merged

dinosaure closed this Oct 12, 2021

Conversation

dinosaure commented Aug 29, 2018

Uh oh!

avsm Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seliopou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seliopou commented Dec 6, 2018

Uh oh!

seliopou commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seliopou Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dinosaure commented Dec 18, 2018

Uh oh!

dinosaure commented Dec 18, 2018

Uh oh!

cfcs Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

dinosaure Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seliopou commented Dec 6, 2018 •

edited

Loading