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

fix test for linux, don't hang after Error writing to socket: Broken … #8559

Closed
wants to merge 1 commit into from

Conversation

rdp
Copy link
Contributor

@rdp rdp commented Dec 6, 2019

…pipe (Errno) ...SSL_do_handshake

Test worked great in OS X, but in Linux somehow even after the client establishes the connection it's not yet "fully" established or something. Either that or packets are just plain dropped when #close is called on a localhost sockets? Not quite sure what was going on there.

Regardless, in linux boxes running the spec suite would freeze/hang at this point. Now it continues and passes.

@rdp
Copy link
Contributor Author

rdp commented Dec 7, 2019

OK seems the problem is a bit deeper than I expected, still investigating...

@rdp
Copy link
Contributor Author

rdp commented Dec 12, 2019

Here's a "righter" fix, working on updating here:

diff --git a/spec/std/openssl/ssl/socket_spec.cr b/spec/std/openssl/ssl/socket_spec.cr
index 1b6675327..9ba587f1f 100644
--- a/spec/std/openssl/ssl/socket_spec.cr
+++ b/spec/std/openssl/ssl/socket_spec.cr
@@ -20,6 +20,27 @@ describe OpenSSL::SSL::Socket do
     end
   end
 
+  it "accepts clients that don't read anything and close the connection" do
+    tcp_server = TCPServer.new(0)
+    server_context, client_context = ssl_context_pair
+    # in tls 1.3, if clients don't read anything and close the connection
+    # the server might still try and send it a ticket, resulting in a pipe failure
+    server_context.disable_session_resume_tickets
+
+    OpenSSL::SSL::Server.open(tcp_server, server_context) do |server|
+      spawn do
+        # the :sync_close portion, as implemented, effects a socket close from the client
+        OpenSSL::SSL::Socket::Client.open(TCPSocket.new(tcp_server.local_address.address, tcp_server.local_address.port), client_context, hostname: "example.com", sync_close: true) do |socket|
+          # doesn't read anything, just write and close connection immediately
+          socket.puts "hello"
+        end
+      end
+
+      client = server.accept # shouldn't raise
+      client.close
+    end
+  end
+
   it "returns the TLS version" do
     tcp_server = TCPServer.new(0)
     server_context, client_context = ssl_context_pair
@@ -39,6 +60,7 @@ describe OpenSSL::SSL::Socket do
   it "closes connection to server that doesn't properly terminate SSL session" do
     tcp_server = TCPServer.new(0)
     server_context, client_context = ssl_context_pair
+    server_context.disable_session_resume_tickets
 
     client_successfully_closed_socket = Channel(Nil).new
     spawn do
diff --git a/src/openssl/lib_ssl.cr b/src/openssl/lib_ssl.cr
index f663c3ea4..455e83c41 100644
--- a/src/openssl/lib_ssl.cr
+++ b/src/openssl/lib_ssl.cr
@@ -206,6 +206,12 @@ lib LibSSL
   # Hostname validation for OpenSSL <= 1.0.1
   fun ssl_ctx_set_cert_verify_callback = SSL_CTX_set_cert_verify_callback(ctx : SSLContext, callback : CertVerifyCallback, arg : Void*)
 
+  # control TLS 1.3 session ticket generation
+  {% if compare_versions(OPENSSL_VERSION, "1.1.1") >= 0 %}
+    fun ssl_ctx_set_num_tickets = SSL_CTX_set_num_tickets(ctx : SSLContext, larg : LibC::SizeT) : Int
+    fun ssl_set_num_tickets = SSL_set_num_tickets(ctx : SSL, larg : LibC::SizeT) : Int
+  {% end %}
+
   {% if compare_versions(OPENSSL_VERSION, "1.1.0") >= 0 %}
     fun tls_method = TLS_method : SSLMethod
   {% else %}
diff --git a/src/openssl/ssl/context.cr b/src/openssl/ssl/context.cr
index cad1934b6..e0eea9e6f 100644
--- a/src/openssl/ssl/context.cr

@rdp
Copy link
Contributor Author

rdp commented Dec 13, 2019

superceded by #8582

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