Migrate all top level uses of ::Digest to OpenSSL::Digest. #39410
Conversation
|
I see related: ruby/ruby#3149 @rhenium does this route look reasonable to you? Or something to be offset to ruby instead. |
|
It would be great for us if ruby/openssl#377 were merged! If not, perhaps we should consider adding our own level of indirection, since ruby/openssl#366 is being considered. |
|
since ruby/openssl#366 is being considered, can't we use |
|
I changed where ever we can for the preferred usage according to the ruby/openssl#366 / Ruby. For 2 cases, I think we should handle separately: https://github.com/rails/rails/blob/22945780bd56daac6b95486815fa1b2cce9676e9/activesupport/lib/active_support/core_ext/digest/uuid.rb#L18 will need a deprecation that uuid should accept string instead of directly passing hash class. https://github.com/rails/rails/blob/d5bd6e91b198420acddd649f34e9fd8bd73cd566/activesupport/lib/active_support/digest.rb#L8 here we can do soft-deprecate as well and accept string, or also consider just allowing both string/class. |
…tring.
Before:
Digest::UUID.uuid_from_hash(Digest::SHA1, Digest::UUID::OID_NAMESPACE, "1.2.3")
# => "42d5e23b-3a02-5135-85c6-52d1102f1f00"
After:
Digest::UUID.uuid_from_hash("SHA1", Digest::UUID::OID_NAMESPACE, "1.2.3")
# => "42d5e23b-3a02-5135-85c6-52d1102f1f00"
Related discussions:
ruby/openssl#362
ruby/openssl#304
rails#39410 (comment)
vipulnsward
force-pushed
the
replace-digest-openssl
branch
from
96ed08e
to
6495fa0
Compare
…cated in favour of String.
Before:
ActiveSupport::Digest.hash_digest_class = OpenSSL::Digest::SHA256
After:
ActiveSupport::Digest.hash_digest_class = "SHA256"
Related discussions:
ruby/openssl#362
ruby/openssl#304
rails#39410
Sibling PR: rails#39504
|
Related on this as it gets adopted to more projects: rubocop/rubocop#7950 in anticipation of deprecation. |
More information here: https://bugs.ruby-lang.org/issues/13681 openssl/openssl@65300dc From OpenSSL Changelog which is hard to link: h ttps://github.com/openssl/openssl/blob/ccb8f0c87eb28d55a6607504f2fbf1be94836c49/CHANGES.md#L5106 > Experimental multi-implementation support for FIPS capable OpenSSL. When in FIPS mode the approved implementations are used as normal, when not in FIPS mode the internal unapproved versions are used instead. This means that the FIPS capable OpenSSL isn't forced to use the (often lower performance) FIPS implementations outside FIPS mode. > Steve Henson So the issue is on Ruby's internal version we don't have a gradual fallback. But using the openssl version > 1.0.1l and 1.0.2, openssl uses internal implementation to maintain FIPS compat One concern to move to OpenSSL::Digest could be that top level ::Digest is ruby implementation and not OpenSLL, but given we use the OpenSSL versions all over, I think its save to move to use OpenSSL versions at remaining places Migrate all non-OpenSSL digest class references to OpenSSL versions Import openssl instead of digest variants on all files OpenSSL::Digest::SHA2 => OpenSSL::Digest::SHA256 Use the preferred ways of initializing Digest classes or use toplevel class methods instead of algorithm constants in Digest class Details: ruby/openssl#304 ruby/openssl#362
vipulnsward
force-pushed
the
replace-digest-openssl
branch
from
6495fa0
to
94ff09b
Compare
There was a problem hiding this comment.
Cop author here, looks good to me but you may want to add the cop to the Rubocop config as well 😉
Not strictly necessary but you can clean things up a little further in a few places by using the string algorithm instead of a OpenSSL::Digest instance when calling digest or hexdigest.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Summary
Migrate all top level uses of
::DigesttoOpenSSL::Digest.Fixes #38791
More information here:
https://bugs.ruby-lang.org/issues/13681
openssl/openssl@65300dc
From OpenSSL Changelog which is hard to link:
https://github.com/openssl/openssl/blob/ccb8f0c87eb28d55a6607504f2fbf1be94836c49/CHANGES.md#L5106
So the issue is on Ruby's internal version we don't have a gradual fallback. But using the openssl version > 1.0.1l and 1.0.2, openssl uses internal implementation to maintain FIPS compat
One concern to move to OpenSSL::Digest could be that top level ::Digest is ruby implementation and not OpenSLL, but given we use the OpenSSL versions all over, I think its save to move to use OpenSSL versions at remaining places