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

Bump OpenSSL gem requirement to 2.2 #324

Merged
merged 3 commits into from
Mar 20, 2022
Merged

Bump OpenSSL gem requirement to 2.2 #324

merged 3 commits into from
Mar 20, 2022

Conversation

bdewater
Copy link
Collaborator

This allows us to lower the amount of test permutations on Travis and take advantage of the built-in helper functions for constant time comparison and working with certificate extensions.

This allows us to lower the amount of test permutations on Travis and take advantage of the built-in helper functions for constant time comparison and working with certificate extensions.
Copy link
Member

@brauliomartinezlm brauliomartinezlm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you! 🍻

Copy link
Contributor

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Not sure about making us incompatible with openssl rubygem v2.0 and v2.1, these are the third-to-last and second-to-last versions of that gem.

There may be ruby apps out there depending on openssl v2.1 (or v2.0) for some reason (or using gems that depend on those), and this would render webauthn unusable for them, right?

If so, I think I'd prefer to wait more time before we start dropping support for these version... (as newer openssl versions come out)

Sorry I didn't voiced this concern earlier.

@grzuy
Copy link
Contributor

grzuy commented Jul 6, 2020

Maybe we can drop v2.0 and keep at least two minor versions compatible?

Thoughts?

@bdewater
Copy link
Collaborator Author

bdewater commented Jul 6, 2020

There may be ruby apps out there depending on openssl v2.1 (or v2.0) for some reason (or using gems that depend on those), and this would render webauthn unusable for them, right?

For gems: I've checked https://rubygems.org/gems/openssl/reverse_dependencies and besides a couple of gems that are requiring 2.1.2 specifically for some reason, the rest either is OK with any OpenSSL version or any 2.x version.

For apps: at my employer there's a mix of 2.1.2 and 2.2 in use. I checked the 2.1.2 apps and none of them have another gem depending on OpenSSL (i.e. comes from the app Gemfile), nor are they locked to 2.1.2 for specific reason according to git history. https:/ruby/openssl/blob/master/History.md also shows no reason not to upgrade to 2.2 IMO.

In short, I don't believe this to be a real issue.

If so, I think I'd prefer to wait more time before we start dropping support for these version... (as newer openssl versions come out)

Maybe we can drop v2.0 and keep at least two minor versions compatible?

We could, although I don't think dropping 2.0 makes much sense as the real benefits are in 2.2 - which I should've explained at the start:

We could wait until OpenSSL gem 3.0 is out and bump the requirement then.

@grzuy
Copy link
Contributor

grzuy commented Jul 8, 2020

There may be ruby apps out there depending on openssl v2.1 (or v2.0) for some reason (or using gems that depend on those), and this would render webauthn unusable for them, right?

For gems: I've checked https://rubygems.org/gems/openssl/reverse_dependencies and besides a couple of gems that are requiring 2.1.2 specifically for some reason, the rest either is OK with any OpenSSL version or any 2.x version.

Great.

For apps: at my employer there's a mix of 2.1.2 and 2.2 in use. I checked the 2.1.2 apps and none of them have another gem depending on OpenSSL (i.e. comes from the app Gemfile), nor are they locked to 2.1.2 for specific reason according to git history. https:/ruby/openssl/blob/master/History.md also shows no reason not to upgrade to 2.2 IMO.

In short, I don't believe this to be a real issue.

Gotcha.

If so, I think I'd prefer to wait more time before we start dropping support for these version... (as newer openssl versions come out)

Maybe we can drop v2.0 and keep at least two minor versions compatible?

We could, although I don't think dropping 2.0 makes much sense as the real benefits are in 2.2 - which I should've explained at the start:

* Built-in `secure_compare` allowing us to drop a dependency:

* PKCS8 support, which will be a requirement for storing EdDSA keys (#276)

We could wait until OpenSSL gem 3.0 is out and bump the requirement then.

Right, thanks for the extra context.

I think I still prefer to err on the side of caution with this case...

Can we split this in two steps?

  • One that drops openssl v2.0, which can drop all the "PSS avialable" conditional code, right?
  • Then separate PR for dropping openssl v2.1 - which would be good to have it ready, but I think I'd pefer to wait more time before merging and releasing. Probably not before openssl v3.0 is out.

By the way, thank you for the effort you've been putting in improving in https:/ruby/openssl also 😃 💯

@grzuy
Copy link
Contributor

grzuy commented Oct 17, 2020

  • One that drops openssl v2.0, which can drop all the "PSS avialable" conditional code, right?

Just did here: 2cbdaad :-)

@grzuy
Copy link
Contributor

grzuy commented Mar 14, 2021

Pushed a few commits to keep this is up to date 🙂

  • Then separate PR for dropping openssl v2.1 - which would be good to have it ready, but I think I'd pefer to wait more time before merging and releasing. Probably not before openssl v3.0 is out.

Still think a good idea to wait for openssl v3.0 to be out before merging though.

@jeremyevans
Copy link

openssl 3.0.0 gem has been released and is part of Ruby 3.1. It would be best to get a release out that doesn't require downgrading openssl on Ruby 3.1. FWIW, this wouldn't be a problem if you switched to open ended dependency versions.

@loqs
Copy link

loqs commented Feb 28, 2022

FWIW, this wouldn't be a problem if you switched to open ended dependency versions.

openssl 3.0 the gem when built with OpenSSL 3.0 the library removes methods used by webauthn-ruby directly such as
OpenSSL::PKey::EC#generate_key and indirectly through cose such as OpenSSL::PKey::RSA#set_key
generate_key can be replaced by generate, I believe there is currently no direct replacement for set_key.

@jeremyevans
Copy link

FWIW, this wouldn't be a problem if you switched to open ended dependency versions.

openssl 3.0 the gem when built with OpenSSL 3.0 the library removes methods used by webauthn-ruby directly such as OpenSSL::PKey::EC#generate_key and indirectly through cose such as OpenSSL::PKey::RSA#set_key generate_key can be replaced by generate, I believe there is currently no direct replacement for set_key.

With LibreSSL 3.5.0 (latest release), and repository checkouts of tpm-key_attestation, openssl-signature_algorithm, cose-ruby, and webauthn-ruby, it appears webauthn_ruby works fine with openssl gem 3.0.0 (all of Rodauth's webauthn tests pass with it). It seems unfair to non-OpenSSL-3.0 users to enforce an unnecessary restriction on the openssl gem version. Would you consider documenting the issue and removing the restriction? Or are you worried that there are too many users that don't read the documentation and that such a change will cause a support burden?

@brauliomartinezlm
Copy link
Member

brauliomartinezlm commented Mar 20, 2022

A long time has passed and few releases of OpenSSL. I think it's worth evaluating supporting OpenSSL 3 but let's do it separately after we bump to 2.2 and drop support for the lower versions.

I'm gonna merge this and release 2.5.1 with it @bdewater

Thank you all that chimed in this PR and shared thoughts. We can create another issue/PR to followup on supporting 3.x and loosening the dependency.

@brauliomartinezlm brauliomartinezlm merged commit 4d76b1e into master Mar 20, 2022
@brauliomartinezlm brauliomartinezlm deleted the openssl-22 branch March 20, 2022 15:40
@loqs loqs mentioned this pull request Apr 5, 2022
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.

5 participants