Project

General

Profile

Actions

Bug #13718

closed

openssl: Reading PEM/DER from an IO object fails on mswin Ruby

Added by MSP-Greg (Greg L) over 6 years ago. Updated over 6 years ago.

Status:
Third Party's Issue
Assignee:
-
Target version:
-
ruby -v:
ruby 2.5.0dev (2017-07-05) [x64-mswin64_140]
[ruby-core:81919]

Description

I recently decided to locally build/test mswin based on VS2015/140. The current Ruby Appveyor testing is done using VS2013/120. I did so since using Appveyor on my Ruby fork was wasteful and time consuming, especially if the problem was testing related and didn't require a new build.

My choice of VS was based on what I had installed, and also on the fact that OpenSSL is now testing on Appveyor using only 140 for both master and OpenSSL_1_1_0-stable. FYI, OpenSSL_1_0_2-stable tests against several VS versions.

My mswin build is using a custom OpenSSL 1.1.0f package I built, using a script based on the OpenSSL script and info at Building OpenSSL with Visual Studio. Both the package and Ruby OpenSSL pass all tests. For my MinGW builds/tests, I also build a package using 1.1.0f, and it also passes all tests in both the package and Ruby.

When running test-all, I had silent SEGV failures on the net tests - test_ftp.rb, test_imap.rb, and test_smtp.rb. All three currently use the following code for some tests:

ctx = OpenSSL::SSL::SSLContext.new
ctx.ca_file = CA_FILE
ctx.key = File.open(SERVER_KEY) { |f|
  OpenSSL::PKey::RSA.new(f)
}
ctx.cert = File.open(SERVER_CERT) { |f|
  OpenSSL::X509::Certificate.new(f)
}

From the docs, both OpenSSL::PKey::RSA.new and OpenSSL::X509::Certificate.new expect a string, but they're being passed a File instance. Also, why create a block when one isn't needed?

Obviously, tests written this way currently pass on Travis, Appveyor (using 120), and all of the MinGW builds I have done.

But, they do not pass using VS2015/140. I could push a PR for changes to the test files, but I thought I'd file an issue first. I added a utils.rb file to remove the repetition, and used the following:

ctx = OpenSSL::SSL::SSLContext.new
ctx.ca_file = CA_FILE
ctx.key  = OpenSSL::PKey::RSA.new( File.read(SERVER_KEY) )
ctx.cert = OpenSSL::X509::Certificate.new( File.read(SERVER_CERT) )

Any thoughts on this odd behavior?


Files

test-net_openssl_ctx.patch (11.5 KB) test-net_openssl_ctx.patch MSP-Greg (Greg L), 07/06/2017 03:10 PM

Updated by rhenium (Kazuki Yamaguchi) over 6 years ago

  • Subject changed from net/* tests using OpenSSL::PKey::RSA and OpenSSL::X509::Certificate to openssl: Reading PEM/DER from an IO object fails on mswin Ruby
  • Status changed from Open to Third Party's Issue

MSP-Greg (Greg L) wrote:

When running test-all, I had silent SEGV failures on the net tests - test_ftp.rb, test_imap.rb, and test_smtp.rb. All three currently use the following code for some tests:

https://github.com/ruby/openssl/issues/128 is tracking this bug.

ctx = OpenSSL::SSL::SSLContext.new
ctx.ca_file = CA_FILE
ctx.key = File.open(SERVER_KEY) { |f|
  OpenSSL::PKey::RSA.new(f)
}
ctx.cert = File.open(SERVER_CERT) { |f|
  OpenSSL::X509::Certificate.new(f)
}

From the docs, both OpenSSL::PKey::RSA.new and OpenSSL::X509::Certificate.new expect a string, but they're being passed a File instance. Also, why create a block when one isn't needed?

This is a separate doc issue. I'll will fix them.

Updated by MSP-Greg (Greg L) over 6 years ago

rhenium (Kazuki Yamaguchi) wrote:

This is a separate doc issue. I'll will fix them.

Thank you. Having looked over a great deal of the test code, I feel that the net test code could benefit from shared code and constants for the ctx creation, along with a normal use of a string, rather than an IO.

If IO functionality needs to be tested, it should be done in the OpenSSL tests. The net tests should use the simplest code possible for other functionality, as in OpenSSL. Initializing a block and creating a File/IO object when only File.read is required seems odd. When tests are failing, I don't want to see 'this is more complex, but you can do this!' examples.

If you or anyone else shares my feelings, attached is a patch file for test changes.

Actions

Also available in: Atom PDF

Like0
Like0Like0