Lookup digests/cipher by name instead of constants #362
Conversation
This is not present in the referenced files anymore, and not useful to most users
bdewater
force-pushed
the
no-digest-cipher-constants
branch
from
6f86339
to
6218949
Compare
|
Great work. |
|
@ioquatix Can you please make an explicit merge commit when merging a Pull Request, rather than using the "rebase and merge" button? Doing so makes it hard to handle when we somehow have to revert a change consisting of a series of commits. Also, it's inconvenient that it's not possible to find the Pull Request where a commit was originally created without using the web browser. Personally, I don't see a benefit that outweighs those downsides. |
|
The other option is squash commits, which solve all those problems and probably would have made more sense for this work, given that some commits are just fixups of previous commits. I don't mind following what method you prefer, so just let me know - merge or squash? |
|
To nitpick, I think this Pull Request could be 3 commits, with the 4th commit being squashed into 1st one. However, I wouldn't mix other three changes: one updates use of Digest names, another updates Ciphers, and the other removes an outdated section in the documentation. They should remain separate. I prefer using the classic merge consistently. Although "Squash and merge" itself is perfectly fine when a Pull Request contains a single change, I don't think it's a good idea to use both. |
|
Understood, thanks for the detailed feedback, I'll follow your guide for this repo :) |
…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)
…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
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
As discussed in #304 this updates documentation and tests to show the new preferred usage. I also took the liberty of updating the Digest documentation a little.
The same constants as today in version 2.1.2 work for backwards compatibility, minus
OpenSSL::Digest::MDCS2as that was removed in 11aba35 already.I can contribute a PR to Rubocop for autocorrecting these changes if this is merged.