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 issue with OpenSSL 3.0 #294

Merged
merged 2 commits into from Oct 6, 2022
Merged

Fix issue with OpenSSL 3.0 #294

merged 2 commits into from Oct 6, 2022

Conversation

ahukkanen
Copy link
Contributor

There is still a compatibility issue in the Bulletin Board with OpenSSL 3.0.

The comment is stating that after jwt/ruby-jwt#375 is merged and released, the extra stuff in the import code is no longer needed which seems to make sense looking at the changes in that PR. These changes became part of jwt-2.2.3 (2021-04-19), so these should no longer be needed.

When running the current code under OpenSSL 3.0, you will see the following exception:

     @bulletin_board ||= Decidim::BulletinBoard::Client.new
     
     OpenSSL::PKey::PKeyError:
       rsa#set_key= is incompatible with OpenSSL 3.0
     # /home/ahukkanen/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/decidim-bulletin_board-0.24.0/lib/decidim/bulletin_board/jwk_utils.rb:19:in `set_key'
     # /home/ahukkanen/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/decidim-bulletin_board-0.24.0/lib/decidim/bulletin_board/jwk_utils.rb:19:in `import_private_key'
     # /home/ahukkanen/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/decidim-bulletin_board-0.24.0/lib/decidim/bulletin_board/settings.rb:13:in `initialize'
     # /home/ahukkanen/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/decidim-bulletin_board-0.24.0/lib/decidim/bulletin_board/client.rb:26:in `new'
     # /home/ahukkanen/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/decidim-bulletin_board-0.24.0/lib/decidim/bulletin_board/client.rb:26:in `initialize'

Therefore, we should remove this code as stated in the comment and let jwt itself handle this part.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2022

Codecov Report

Merging #294 (7e53020) into develop (1b4db9e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #294   +/-   ##
========================================
  Coverage    69.57%   69.57%           
========================================
  Files           96       96           
  Lines         1630     1630           
========================================
  Hits          1134     1134           
  Misses         496      496           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ahukkanen
Copy link
Contributor Author

We should probably also test this under OpenSSL 3, so we would catch this kind of issues in the CI.

I think the main problem is that the base Ruby image we are relying on is based on Debian Bullseye:
https://github.com/docker-library/ruby/blob/41208aad6764925f0f082a695a076f7f39af1233/3.1/bullseye/Dockerfile#L7

And Bullseye ships with OpenSSL 1.1 by default:
https://packages.debian.org/bullseye/openssl

Currently we would either need to install OpenSSL 3 manually to the container when we build it or build an alternative container for testing. It's a bit cumbersome... But Debian Bookworm should be released shortly looking at their release history, so we could just wait for that and then wait the Ruby Docker containers to be released that are based on that release. Then just swap the base Ruby image to e.g. ruby:3.1.2-bookworm (if they'll ever release that, we'll see).

I suggest currently testing it manually and after Bookworm is released, see again what our options are.

@ahukkanen
Copy link
Contributor Author

The least that I could do is to run the client specs under Ubuntu 22.04 as of 7e53020.

Running the E2E tests under OpenSSL 3 would be harder as explained above. I think the "easiest" way around that would be to uninstall the base image's openssl package and build OpenSSL 3 from source but that may have unexpected consequences in other packages or the Ruby native modules that were built using OpenSSL 1.

@ahukkanen
Copy link
Contributor Author

@andreslucena Forgot to mention in the maintainers meeting but I'd appreciate to get this also merged so that I can release new version of the bulletin board ruby client with OpenSSL 3 support (the current release is broken under OpenSSL).

To test this:

  1. Run the ruby client specs at develop under Ubuntu 22.04 -> Expect to see lots of errors
  2. Run the ruby client specs at this PR branch under Ubuntu 22.04 -> Expect to see green

This is not urgent regarding the 0.27 release but we need this to fix the issue for the next 0.28 release. Just wanted to ping to you that forgot to mention about this one.

@andreslucena
Copy link
Member

The comment is stating that after jwt/ruby-jwt#375 is merged and released, the extra stuff in the import code is no longer needed which seems to make sense looking at the changes in that PR.

I found out this TODO last year. I left an issue, so we don't miss it, so this will fix #248

@andreslucena andreslucena linked an issue Oct 6, 2022 that may be closed by this pull request
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏽

Environment

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Pop
Description:	Pop!_OS 22.04 LTS
Release:	22.04
Codename:	jammy
$ openssl version
OpenSSL 3.0.2 15 Mar 2022 (Library: OpenSSL 3.0.2 15 Mar 2022)

Without this PR

$ bundle exec rspec  ./spec/decidim/bulletin_board/jwk_utils_spec.rb

Randomized with seed 51997

Decidim::BulletinBoard::JwkUtils
  when working with a valid private key
    can do a private export (FAILED - 1)
    returns the right thumbprint
    is expected to be private key {:d=>"Eg9UYFs2a2Yq9pu0k9SKJm_UlRy-Dsj5HnIcwvHro5uzk2qH0xBg76HpgNhm5VQQB9oiXTlwPVDefct-efGvEL4bxN-xBsl...Xy1JmwHHEgaxj_WAtMG_elwXDJeHoLwcsQsp7JhIBacgpfFvfY9Uo88Rx3RG4IYtRX8GfV8QDX5cgsmBsKq2wVrIEWkH_H5F8U"}
  when working with an invalid private key
    returns the right thumbprint
    can't do a private export
    is expected not to be private key {:alg=>"RS256", :e=>"AQAB", :kid=>"2011-04-29", :kty=>"RSA", :n=>"0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEp...D08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQ-G_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw"}

Failures:

  1) Decidim::BulletinBoard::JwkUtils when working with a valid private key can do a private export
     Failure/Error: jwk.keypair.set_key(decode_open_ssl_bn(json[:n]), decode_open_ssl_bn(json[:e]), decode_open_ssl_bn(json[:d]))
     
     OpenSSL::PKey::PKeyError:
       rsa#set_key= is incompatible with OpenSSL 3.0
     # ./lib/decidim/bulletin_board/jwk_utils.rb:19:in `set_key'
     # ./lib/decidim/bulletin_board/jwk_utils.rb:19:in `import_private_key'
     # ./spec/decidim/bulletin_board/jwk_utils_spec.rb:46:in `block (3 levels) in <top (required)>'
     # ./spec/decidim/bulletin_board/jwk_utils_spec.rb:55:in `block (3 levels) in <top (required)>'

Finished in 0.00888 seconds (files took 0.81133 seconds to load)
6 examples, 1 failure

Failed examples:

rspec ./spec/decidim/bulletin_board/jwk_utils_spec.rb:54 # Decidim::BulletinBoard::JwkUtils when working with a valid private key can do a private export

Randomized with seed 51997

After the PR

$ bundle exec rspec  ./spec/decidim/bulletin_board/jwk_utils_spec.rb

Randomized with seed 51408

Decidim::BulletinBoard::JwkUtils
  when working with an invalid private key
    can't do a private export
    returns the right thumbprint
    is expected not to be private key {:alg=>"RS256", :e=>"AQAB", :kid=>"2011-04-29", :kty=>"RSA", :n=>"0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEp...D08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQ-G_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw"}
  when working with a valid private key
    returns the right thumbprint
    can do a private export
    is expected to be private key {:d=>"Eg9UYFs2a2Yq9pu0k9SKJm_UlRy-Dsj5HnIcwvHro5uzk2qH0xBg76HpgNhm5VQQB9oiXTlwPVDefct-efGvEL4bxN-xBsl...Xy1JmwHHEgaxj_WAtMG_elwXDJeHoLwcsQsp7JhIBacgpfFvfY9Uo88Rx3RG4IYtRX8GfV8QDX5cgsmBsKq2wVrIEWkH_H5F8U"}

Finished in 0.01251 seconds (files took 0.83626 seconds to load)
6 examples, 0 failures

Randomized with seed 51408

@andreslucena andreslucena added dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code labels Oct 6, 2022
@andreslucena andreslucena merged commit 48d3a4f into develop Oct 6, 2022
@andreslucena andreslucena deleted the fix/openssl-3-support branch October 6, 2022 11:25
@ahukkanen ahukkanen mentioned this pull request Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ruby-jwt to v2.2.3 (at least)
3 participants