search

cedarcode / webauthn-ruby

WebAuthn ruby server library ― Make your Ruby/Rails web server become a conformant WebAuthn Relying Party
https://rubygems.org/gems/webauthn
MIT License
649 stars 53 forks source link

Bump OpenSSL gem requirement to 2.2 #324

Closed bdewater closed 2 years ago

bdewater commented 4 years ago

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.

grzuy commented 4 years ago

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

Thoughts?

bdewater commented 4 years ago

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://github.com/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 commented 4 years ago

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://github.com/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?

By the way, thank you for the effort you've been putting in improving in https://github.com/ruby/openssl also :smiley: :100:

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

Just did here: https://github.com/cedarcode/webauthn-ruby/commit/2cbdaad5965416b21a718430a0cbea93f110a511 :-)

grzuy commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.