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

Strange behavior on OpenSSL::SSL::SSLSocket#tmp_key, wrong class returned #360

Closed
aeris opened this issue Apr 13, 2020 · 7 comments
Closed

Comments

@aeris
Copy link
Contributor

aeris commented Apr 13, 2020

According to unit tests, OpenSSL::SSL::SSLSocket#tmp_key is supposed to return OpenSSL::PKey::EC or OpenSSL::PKey::DH.

I add some debug on unit test:

$ git diff test/test_ssl.rb 
diff --git a/test/test_ssl.rb b/test/test_ssl.rb
index 060c1f1..9d2acc9 100644
--- a/test/test_ssl.rb
+++ b/test/test_ssl.rb
@@ -1305,6 +1305,7 @@ end
         ctx.ssl_version = :TLSv1_2
         ctx.ciphers = "EDH"
         server_connect(port, ctx) { |ssl|
+          p "#{__FILE__}:#{__LINE__} #{ssl.tmp_key.class}"
           assert_instance_of OpenSSL::PKey::DH, ssl.tmp_key
         }
       end
@@ -1320,6 +1321,7 @@ end
         ctx = OpenSSL::SSL::SSLContext.new
         ctx.ciphers = "DEFAULT:!kRSA:!kEDH"
         server_connect(port, ctx) { |ssl|
+          p "#{__FILE__}:#{__LINE__} #{ssl.tmp_key.class}"
           assert_instance_of OpenSSL::PKey::EC, ssl.tmp_key
           ssl.puts "abc"; assert_equal "abc\n", ssl.gets
         }

And then run them, all are good and the expected class are correctly seen:

$ RBENV_VERSION=2.6.6 rake test |& rg test_ssl
"ruby-openssl/test/test_ssl.rb:1308 OpenSSL::PKey::DH"
"ruby-openssl/test/test_ssl.rb:1324 OpenSSL::PKey::EC"
$ RBENV_VERSION=2.3.8 rake test |& rg test_ssl
"ruby-openssl/test/test_ssl.rb:1308 OpenSSL::PKey::DH"
"ruby-openssl/test/test_ssl.rb:1324 OpenSSL::PKey::EC"

But when executed on real code

#!/usr/bin/env ruby
require 'openssl'
require 'socket'

context = OpenSSL::SSL::SSLContext.new :TLSv1_2
tcp_socket = TCPSocket.new 'imirhil.fr', 443
ssl_client = OpenSSL::SSL::SSLSocket.new tcp_socket, context
ssl_client.sync_close = true
ssl_client.connect
puts ssl_client.tmp_key
ssl_client.puts "GET / HTTP/1.0\n\n"
puts ssl_client.gets
ssl_client.close

We get a bare OpenSSL::PKey::PKey only on 2.6, not on 2.3 🤔

$ RBENV_VERSION=2.3.8 ./test.rb
#<OpenSSL::PKey::EC:0x00007f535ee2a2a8>
$ RBENV_VERSION=2.6.6 ./test.rb
#<OpenSSL::PKey::PKey:0x00007f5df9f8eec8>

I don't understand this behavior and why unit test result are different from real code result, and why different behavior from one ruby version to another…

@aeris aeris changed the title Strange behavior on OpenSSL::SSL::SSLSocket#tmp_key Strange behavior on OpenSSL::SSL::SSLSocket#tmp_key, wrong class returned Apr 13, 2020
@rhenium
Copy link
Member

rhenium commented Apr 13, 2020

OpenSSL::SSL::SSLSocket#tmp_key will return the generic OpenSSL::PKey when it's representable neither by OpenSSL::PKey::DH nor by OpenSSL::PKey::EC. For example, X25519 falls in this category.

The test code disables X25519 explicitly (commit a001911).

The difference between Ruby 2.3 and 2.6 must be the OpenSSL library version linked with. X25519 was first supported by OpenSSL 1.1.0.

@aeris
Copy link
Contributor Author

aeris commented Apr 13, 2020

I got the same result with ECDHE cipher… 🤔
ECDHE-ECDSA-CHACHA20-POLY1305 => OpenSSL::PKey::PKey (must be EC)
ECDHE-RSA-AES128-GCM-SHA256 => OpenSSL::PKey::PKey (must be EC)

@rhenium
Copy link
Member

rhenium commented Apr 13, 2020

TLS's ECDH key exchange can use X25519, but OpenSSL treats it separately in the EVP layer.

To prevent X25519 from being chosen (to set the elliptic_curves extension in the TLS Client Hello, which by default includes X25519), you can use OpenSSL::SSL::SSLContext#ecdh_curves=.

@aeris
Copy link
Contributor Author

aeris commented Apr 13, 2020

Oh ok, thanks, will do more test on my side to check this.

@aeris
Copy link
Contributor Author

aeris commented Apr 13, 2020

Seems working!
Is there any way to know the curve associated to an OpenSSL::PKey::PKey instance, without waiting for #329 inclusion? Or have I to use an heuristic like "if not EC nor DH, it x55119" at the moment?

@rhenium
Copy link
Member

rhenium commented Apr 20, 2020

At the moment, yes. There is no way to tell what type of key is an OpenSSL::PKey::PKey holding. This seems a missing feature. #364 adds #oid (and overrides #inspect).

@rhenium
Copy link
Member

rhenium commented Apr 21, 2020

I'm closing this issue. With #364, the pkey type is now distinguished. Useful operations on them are to be implemented by #329.

@rhenium rhenium closed this as completed Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants