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

[WIP] Support OpenSSL 3.0 #399

Closed
wants to merge 4 commits into from
Closed

Conversation

rhenium
Copy link
Member

@rhenium rhenium commented Aug 21, 2020

A collection of patches to support OpenSSL 3.0 API.

Currently, as of 2021-05-25, it compiles with OpenSSL's master branch (3.0.0-alpha17), but many things are known to be broken and test suite does not pass.

See also the tracking issue #369 (Support OpenSSL 3.0) for the summary.

These test failures are as of 2020-08, which may or may not be relevant today.

Test suite does not pass yet - some are due to unresolved issues in OpenSSL, which have open issues on openssl/openssl:

  • OpenSSL::PKey.generate_key with HMAC pkey
  • OpenSSL::PKey.read with a passphrase with a null character in the middle

I haven't dug into others, yet:

  • OpenSSL::PKey::PKey.sign_raw against RSA with 'none' padding
  • test_connect_certificate_verify_failed_exception_message(OpenSSL::TestSSL)
  • test_post_connection_check(OpenSSL::TestSSL)
  • test_ciphers(OpenSSL::TestCipher)
  • test_reset(OpenSSL::TestCipher)
  • test_verify_callback(OpenSSL::TestX509Store)

@pvalena
Copy link

pvalena commented May 21, 2021

I've tried to apply this PR on top of Ruby 3.0.1, and I'm experiencing a segfault on running test suite (failed when reading key). Any ideas how to fix that? Is that still expected?

I'll appreciate any help.

@pvalena pvalena mentioned this pull request May 24, 2021
@junaruga
Copy link
Member

I assume that's why this PR is WIP. I think it's better to share why you want to run Ruby on the OpenSSL 3.0 "right now" without waiting the stable version if it is possible. That's helpful for people to know the proper context. As your colleague working in the same company, I know you are trying to build Ruby with OpenSSL 3.0 "right now". I think a possible solution might be to apply a patch to skip related logic by #ifdef or something.

I think the steps to support OpenSSL 3.0 for Ruby is like this. I think what you can do to accelerate the steps is to contribute to step 1 first.

  1. Test ruby/openssl repo with OpenSSL 3.0.
  2. Test ruby/ruby repo porting the lateted ruby/openssl version supporting it.

As a reference, this repo still does not have an openssl 3.0 case.

- openssl-1.0.2u # EOL
- openssl-1.1.0l # EOL
- openssl-1.1.1j

In my humble opinion, it might be also helpful for you to provide a reproductive script such as by using this repo's CI adding the OpenSSL 3.0 or using a container. Otherwise there might not be many things for people to help you here.

@rhenium
Copy link
Member Author

rhenium commented May 25, 2021

The segfault is likely due to OSSL_STORE_attach()'s function signature having been changed in OpenSSL master in the last month. I expect other parts of OpenSSL::PKey can also explode, as the underlying EVP_PKEY structure was made immutable while we've provided "destructive" methods on it. The change happened earlier this year.

Yes, this is a work-in-progress and is not really ready. Please note that OpenSSL 3.0 is still in an alpha release and has not reached its feature freeze.

@pvalena
Copy link

pvalena commented May 25, 2021

I assume that's why this PR is WIP. I think it's better to share why you want to run Ruby on the OpenSSL 3.0 "right now" without waiting the stable version if it is possible. That's helpful for people to know the proper context. As your colleague working in the same company, I know you are trying to build Ruby with OpenSSL 3.0 "right now". I think a possible solution might be to apply a patch to skip related logic by #ifdef or something.

It's not that I need it working "right now" (I'm not sure where did you get that impression). I'm trying to apply it now, for testing purposes, WRT Centos Stream 9.

You're right, the PR is WIP, but that does not mean it's "development behind closed doors", right? I was trying to get the actual (usability) status, using combination of several PRs, and additional modifications, applied it to currently released Ruby (not a development version), as we plan to use it in the future.

I may have applied the patch in a wrong way as well, so that's the part I'd be glad to get any feedback for.

I think the steps to support OpenSSL 3.0 for Ruby is like this. I think what you can do to accelerate the steps is to contribute to step 1 first.

1. Test ruby/openssl repo with OpenSSL 3.0.

2. Test ruby/ruby repo porting the lateted ruby/openssl version supporting it.

As a reference, this repo still does not have an openssl 3.0 case.

- openssl-1.0.2u # EOL
- openssl-1.1.0l # EOL
- openssl-1.1.1j

Jun, if You think it's good to engage in upstream development, feel free to do it. I do not think it's mandatory to be able to have a conversation (give + receive feedback). I'll probably be able to engage more in upstream development in the future, but I'm afraid it's out of scope for me, currently.

In my humble opinion, it might be also helpful for you to provide a reproductive script such as by using this repo's CI adding the OpenSSL 3.0 or using a container. Otherwise there might not be many things for people to help you here.

@pvalena
Copy link

pvalena commented May 25, 2021

The segfault is likely due to OSSL_STORE_attach()'s function signature having been changed in OpenSSL master in the last month. I expect other parts of OpenSSL::PKey can also explode, as the underlying EVP_PKEY structure was made immutable while we've provided "destructive" methods on it. The change happened earlier this year.

Thank you!

Yes, this is a work-in-progress and is not really ready. Please note that OpenSSL 3.0 is still in an alpha release and has not reached its feature freeze.

Yes, AFAICT this is part of the "alpha" compatibility / readiness early testing (not directly by me), as we're encouraged to give feedback as well.

@junaruga
Copy link
Member

You're right, the PR is WIP, but that does not mean it's "development behind closed doors", right?

Right. But I meant if you explain more context, it's better to open the door, not to surprise people. People are busy. It's better to think how to do for people to answer your question easily.

I do not think it's mandatory to be able to have a conversation (give + receive feedback). I'll probably be able to engage more in upstream development in the future, but I'm afraid it's out of scope for me, currently.

I just suggested a possible way for you to do. There is no pressure.

@rhenium rhenium force-pushed the ky/openssl-3.0.0 branch 2 times, most recently from ed548e1 to a811750 Compare May 25, 2021 11:33
@rhenium
Copy link
Member Author

rhenium commented May 25, 2021

OSSL_STORE_attach()

The commit using this function (506be78) is now replaced by commit 01e1230. Some additional necessary changes for OpenSSL 3.0 support are also pushed to GitHub.

It compiles with OpenSSL master (if all the warnings are ignored), but more work is still needed for the other part of OpenSSL::PKey. Also, as you can see in the GitHub Actions' output, many test cases are currently failing due to various reasons.

Yes, AFAICT this is part of the "alpha" compatibility / readiness early testing (not directly by me), as we're encouraged to give feedback as well.

Thanks, this is helpful.

@rhenium rhenium changed the base branch from pending to master May 25, 2021 11:52
@pvalena
Copy link

pvalena commented May 26, 2021

OSSL_STORE_attach()

The commit using this function (506be78) is now replaced by commit 01e1230. Some additional necessary changes for OpenSSL 3.0 support are also pushed to GitHub.

Thanks!
I've replaced the commit in my ruby package patch, but I'm still getting a segfault.

I might by able to try with newer openssl (from the package maintainer) later this week. Additionaly, I will try updating the ruby openssl gem to latest github state, to see whether I can achieve better result.

It compiles with OpenSSL master (if all the warnings are ignored), but more work is still needed for the other part of OpenSSL::PKey. Also, as you can see in the GitHub Actions' output, many test cases are currently failing due to various reasons.

Yes, AFAICT this is part of the "alpha" compatibility / readiness early testing (not directly by me), as we're encouraged to give feedback as well.

Thanks, this is helpful.

I'm glad to hear that, and I'll be happy to test further.

@voxik
Copy link

voxik commented Aug 2, 2021

Please note that OpenSSL 3.0 is still in an alpha release and has not reached its feature freeze.

Since Beta1 is already out, can I gently ask for an update? Thx a lot.

@xtkoba
Copy link

xtkoba commented Aug 17, 2021

A test failure due to a behavior change in OpenSSL 3.0.0 (openssl/openssl#16321):

  1) Failure:
OpenSSL::TestHMAC#test_hmac [test/openssl/test_hmac.rb:12]:
<"9294727a3638bb1c13f48ef8158bfc9d"> expected but was
<"47de897f45806cf1a5a78277cd74b5dc">.

A repro for convenience:

require "openssl"
instance = OpenSSL::HMAC.new('key', 'SHA1')
a = instance
b = instance
exit 1 if a != b

@emptyflask
Copy link

OpenSSL 3.0 is now released (and is now the default version on Homebrew)

@voxik
Copy link

voxik commented Oct 11, 2021

I wonder, will the OpenSSL 3.x support be API compatible with older versions? Going through several PRs, it seems that it probably will, but there are so many changes (for better, I like what I have seen so far).

@voxik
Copy link

voxik commented Nov 11, 2021

Testing Puma against OpenSSL 3.x, I observer the following error:

   7) Failure:
TestPumaControlCli#test_control_ssl
[/builddir/build/BUILD/puma-4.3.6/usr/share/gems/gems/puma-4.3.6/test/test_pumactl.rb:170]:
Expected /Command\ stop\ sent\ success/ to match "SSL_read: unexpected
eof while reading\n".

It seems that this is caused by OpenSSL changing its behavior in EOF handling in SSL_read and this is the affected code:

openssl/ext/openssl/ossl_ssl.c

Lines 1834 to 1835 in 8193b73

int nread = SSL_read(ssl, RSTRING_PTR(str), ilen);
switch (ssl_get_error(ssl, nread)) {

The issue is that now SSL_ERROR_SSL error is raised instead of SSL_ERROR_SYSCALL. This was changed by the following commit:

openssl/openssl@d924dbf

Checking this with @beldmit, I was given openssl/openssl/pull/11735 as an example to fix this issue. If I understand it correctly, it should be enough to set the SSL_OP_IGNORE_UNEXPECTED_EOF if available and this should make the SSL_read to return the SSL_ERROR_ZERO_RETURN instead, which is already handled as EOF.

@beldmit
Copy link

beldmit commented Nov 11, 2021

I am not sure it's the best possible fix, but I had to introduce this option for backward compatibility with IIS, if I'm not mistaken.

@beldmit
Copy link

beldmit commented Nov 11, 2021

See also openssl/openssl#11209 (comment)

@voxik
Copy link

voxik commented Nov 11, 2021

This naive patch fixes the Puma issue:

$ git diff
diff --git a/ext/openssl/ossl_ssl.c b/ext/openssl/ossl_ssl.c
index 3b425ca..40e748c 100644
--- a/ext/openssl/ossl_ssl.c
+++ b/ext/openssl/ossl_ssl.c
@@ -1812,6 +1812,10 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock)
     if (!ssl_started(ssl))
         rb_raise(eSSLError, "SSL session is not started yet");
 
+#ifdef SSL_OP_IGNORE_UNEXPECTED_EOF
+    SSL_set_options(ssl, SSL_OP_IGNORE_UNEXPECTED_EOF);
+#endif
+
     ilen = NUM2INT(len);
     if (NIL_P(str))
        str = rb_str_new(0, ilen);

@beldmit since you have mentioned IIS, do you think this should be better handled on server side and therefore should I report this issue to Puma?

@rhenium
Copy link
Member Author

rhenium commented Nov 15, 2021

The issue is that now SSL_ERROR_SSL error is raised instead of SSL_ERROR_SYSCALL. This was changed by the following commit:

openssl/openssl@d924dbf

I wasn't aware of the behavior change. (This seems to be another case test code was missing.)

Personally I've never been a fan of the current behavior of ruby/openssl, to treat unexpected EOF the same as a proper shutdown. This is for backwards compatibility, but it's a protocol error and potentially makes applications vulnerable to TLS truncation attack. IMO, this was something we needed to get rid of sooner or later.

If OpenSSL now natively provides an option to turn on/off the behavior, it may make sense to let user decide.

SSL_OP_IGNORE_UNEXPECTED_EOF

In any case, we should provide a matching constant under OpenSSL::SSL namespace, just as we do for all other SSL_OP_* flags.

@voxik
Copy link

voxik commented Nov 23, 2021

* # Reading DH parameters
* dh = DH.new(File.read('parameters.pem')) # -> dh, but no public/private key yet
* dh.generate_key! # -> dh with public and private key

Should the example above be updated to not use the deprecated #generate_key!?

@rhenium
Copy link
Member Author

rhenium commented Dec 20, 2021

Most of the patches I posted in this draft PR have been merged to master with more polishing:

Our test suite now runs successfully with OpenSSL 3.0.1. However, we still use some deprecated functions in OpenSSL 3.0 and you will see a number of compilation warnings.

The 3 commits remaining in this PR are wrappers for EVP_PKEY_todo() family. These would be nice to have on OpenSSL 3.0.

@Justinzobel
Copy link

How is the progress on this? I'm starting to test Jammy for server deployments and with libssl-dev 3.0.2 it won't compile as it's limited to OpenSSL >= 1.0.1, < 3.0.0

EVP_PKEY_todata() returns all key components contained in an EVP_PKEY
object as an array of OSSL_PARAM. This is useful as the replacement for
low-level API which is used to implement #params.

EVP_PKEY_todata() currently only exists in OpenSSL 3.0.0.
If EVP_PKEY_get_params() family is available, use it to get the key
component values. Otherwise, check the return value of #to_data, which
should be overriden by the subclasses.
@rhenium
Copy link
Member Author

rhenium commented Aug 31, 2023

Closing this PR, changes already landed on master branch two years ago.

The openssl gem version 3.0.0 (released 2021-12) or later is compatible with OpenSSL 3.0.

@rhenium rhenium closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support OpenSSL 3.0
9 participants