Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for Ktls #146

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

add support for Ktls #146

wants to merge 10 commits into from

Conversation

craff
Copy link
Contributor

@craff craff commented Jun 20, 2023

The main interest is not speed and memory (which it will improve), it is the ability to use Unix.read or Unix.write, allowing to share code for both ssl and non ssl socket.

@craff
Copy link
Contributor Author

craff commented Jun 20, 2023

I added some tests that do use Unix.read and Unix.write.

anmonteiro and others added 5 commits June 20, 2023 13:33
* fix: restore OCaml 4.03 compatibility

* wip

* conditionally run tests

* ==

* wip
* fix: restore OCaml 4.03 compatibility

* wip

* conditionally run tests

* ==

* wip
@craff
Copy link
Contributor Author

craff commented Jun 20, 2023

I do not understand the failure under nix, it occurs befose ocaml-ssl is compiled

Nix won't work in active shell sessions until you restart them.
  
  Could not set environment: 150: Operation not permitted while System Integrity Protection is engaged
  Error: Process completed with exit code 150.

@craff
Copy link
Contributor Author

craff commented Jul 7, 2023

Thanks for the review and merge! Are you planning some opam release ?

@anmonteiro
Copy link
Collaborator

I didn't merge or review this PR. I just fixed the Nix build

@craff
Copy link
Contributor Author

craff commented Jul 7, 2023

I didn't merge or review this PR. I just fixed the Nix build

Sorry, I missread the merge of master in ktls and not the reverse.

@craff
Copy link
Contributor Author

craff commented Jul 7, 2023

I have to check that when using ktls, flush is done by close automatically. This would mean that when the ssl connection is established, the same code with Unix.read/write/close will work both on ssl and non ssl connexion. This save some logic in your application.

@anmonteiro
Copy link
Collaborator

You'll also need to debug why the macOS tests are timing out.

@craff
Copy link
Contributor Author

craff commented Jul 8, 2023

The timeout of tests will occur if the server crashes, this is true for all test. May this should be fixed ?
I made the test work if the platform does not support ktls read and/or write. and if ktls is supported, I check compatibility of Unix read and write with Ssl's.

@craff
Copy link
Contributor Author

craff commented Jul 8, 2023

I wonder if a pure ktls ssl binding (without openssl of libssl) would not be nice ? Should work at least on linux and freeBSD.

@craff
Copy link
Contributor Author

craff commented Jul 8, 2023

One last detail, to enable ktls on debian (seems to work ok on ubuntu as the initial test did not loop on the integrated test), one needs "modprobe tls".
For macosx, if openssl is compiled with tls support, a similar thing might be necessary.

Do we want the test to check that ktls is possible when we think the platform supports it and fail if is does not work?

Anyway, I will update the documentation to explain this...

@craff
Copy link
Contributor Author

craff commented Jul 9, 2023

From
https://www.nginx.com/blog/improving-nginx-performance-with-kernel-tls/

ktls should bring a noticeable speed increase... I did not manage to observe any... Strange. If you have some time to check ? Note that TLS1_3 needs fairly recent kernel to do ktls RX. TLS1_2 is ok for both RX and TX with rather old kernel.

Edit: with vegeta (instead of wrk) I see a 4% speed increase with ktls. This is still not much.

May be openssl has made progress since 2021, date of the above page.

Remain the fact we can use Unix module directly.

@craff
Copy link
Contributor Author

craff commented Jul 9, 2023

I decide not to keep ktls in simple_httpd for now. I might retest it when I have access to a big server, but currently it is not worth it.

I move the branch to draft, until I have time to do real benchmark using only openssl.

I will probably to a PR to include heavy load test in open-ssl.

@craff craff marked this pull request as draft July 9, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants