-
Notifications
You must be signed in to change notification settings - Fork 170
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
digest: redefine ::Digest constants with OpenSSL::Digest ones #377
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace the C implementations from the digest standard library with OpenSSL::Digest ones for increased performance. This makes Digest::SHA1 an alias of OpenSSL::Digest::SHA1, for instance. At the time of writing the digest library uses OpenSSL too, if it is detected on compile-time. However, I'm proposing a patch to remove that code path[1]. The digest library currently uses the legacy low-level API of OpenSSL, whose use has been discouraged for years for multiple reasons. While it could also be rewritten to use the newer EVP API, that requires a breaking change in the Digest framework interface. Also, it's not desirable to have two identical code (Digest and OpenSSL::Digest) as part of the standard library. OpenSSL::Digest inherits from Digest::Class and also implements all methods provided by Digest::Base. Therefore, it should be fully compatible from the perspective of features. Note that, however, this is potentially a breaking change if user code extends Digest::Base for some reason. class Digest::Class include Digest::Instance end class Digest::Base < Digest::Class; end class Digest::SHA1 < Digest::Base; end class Digest::SHA256 < Digest::Base; end ... class OpenSSL::Digest < Digest::Class; end [1] ruby/ruby#3149
rhenium
force-pushed
the
ky/digest-digest-shim
branch
from
May 27, 2020 16:46
5db40f5
to
c4b9b4c
Compare
hsbt
pushed a commit
to ruby/ruby
that referenced
this pull request
Dec 2, 2020
The OpenSSL engine of Digest uses the low-level API of OpenSSL, whose use has been discouraged for years for multiple reasons. A long-standing issue on a FIPS-enabled system is that using ::Digest results in crashing the Ruby process, because the low-level API lacks the mechanism to report an error (the policy violation) and thus kills the process as a last resort[1][2]. Also, the upcoming OpenSSL 3.0 will deprecate it for future removal[3]. Compiling with -Wdeprecated-declarations will start to emit warnings. A proper fix for this is to make it use the EVP API instead. This is a non-trivial work as it requires backwards-incompatible changes to the framework interface of Digest::Base and rb_digest_metadata_t. It is more than 15 years ago that the openssl library became part of the standard library. It has implemented the exactly same functionality as OpenSSL::Digest, in fact, as a subclass of Digest::Class. There is not much point in having an identical code in the digest library. Let's just get rid of OpenSSL within digest. This leaves the C implementations and the CommonCrypto engine for Apple systems. A patch is being prepared for the openssl library to provide ::Digest constants for better performance[4]. [1] https://bugs.ruby-lang.org/issues/6946 [2] https://bugs.ruby-lang.org/issues/13681 [3] https://www.openssl.org/docs/OpenSSL300Design.html [4] ruby/openssl#377
hsbt
pushed a commit
to ruby/digest
that referenced
this pull request
Dec 2, 2020
The OpenSSL engine of Digest uses the low-level API of OpenSSL, whose use has been discouraged for years for multiple reasons. A long-standing issue on a FIPS-enabled system is that using ::Digest results in crashing the Ruby process, because the low-level API lacks the mechanism to report an error (the policy violation) and thus kills the process as a last resort[1][2]. Also, the upcoming OpenSSL 3.0 will deprecate it for future removal[3]. Compiling with -Wdeprecated-declarations will start to emit warnings. A proper fix for this is to make it use the EVP API instead. This is a non-trivial work as it requires backwards-incompatible changes to the framework interface of Digest::Base and rb_digest_metadata_t. It is more than 15 years ago that the openssl library became part of the standard library. It has implemented the exactly same functionality as OpenSSL::Digest, in fact, as a subclass of Digest::Class. There is not much point in having an identical code in the digest library. Let's just get rid of OpenSSL within digest. This leaves the C implementations and the CommonCrypto engine for Apple systems. A patch is being prepared for the openssl library to provide ::Digest constants for better performance[4]. [1] https://bugs.ruby-lang.org/issues/6946 [2] https://bugs.ruby-lang.org/issues/13681 [3] https://www.openssl.org/docs/OpenSSL300Design.html [4] ruby/openssl#377
This was referenced Jan 5, 2021
dbussink
added a commit
to dbussink/rails
that referenced
this pull request
Jan 7, 2021
The Digest::SHA2 class implicitly ends up using SHA256 by default, but there's no OpenSSL version of the same class (OpenSSL::Digest::SHA2 does not exist). To make it easier to force OpenSSL for hash usage and not use deprecated OpenSSL APIs from Digest on older Ruby versions before 3.0, an approach like ruby/openssl#377 might be used. That approach won't work with this SHA2 class though, so it shouldn't be used. The discussion in rails#40770 also indicates that in general the intent is to move to OpenSSL constants anyway, so this makes that move a lot easier as well. Lastly, this updates the cache key usage in the Redis cache store. This store seems to be the only one not using the configured ActiveSupport::Digest class like all other cache usage. Switching it from the not recommended SHA2 class to the ActiveSupport one makes it more consistent with the rest. This also updates the cache key name to not call out the implementation of the hash, and updates memcache in a similar way (where it was still referring to MD5 which is not guaranteed to be used for ActiveSupport::Digest).
I think this is a bad idea. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Request for comments: Is it safe to do this? Would there be any issues?
The digest library (::Digest) needs to either drop the OpenSSL backend or be updated to use the EVP API sooner or later, as the underlying OpenSSL functions are being deprecated. I personally like the former since it's a bad idea to have almost identical code in both digest and openssl. They currently both use OpenSSL for a historical reason: digest was a standard library before openssl joined it in 2003.
Replace the C implementations from the digest standard library with
OpenSSL::Digest ones for increased performance. This makes Digest::SHA1
an alias of OpenSSL::Digest::SHA1, for instance.
At the time of writing the digest library uses OpenSSL too, if it is
detected on compile-time. However, I'm proposing a patch to remove that
code path[1]. The digest library currently uses the legacy low-level API
of OpenSSL, whose use has been discouraged for years for multiple
reasons. While it could also be rewritten to use the newer EVP API, that
requires a breaking change in the Digest framework interface. Also, it's
not desirable to have two identical code (Digest and OpenSSL::Digest) as
part of the standard library.
OpenSSL::Digest inherits from Digest::Class and also implements all
methods provided by Digest::Base. Therefore, it should be fully
compatible from the perspective of features. Note that, however, this is
potentially a breaking change if user code extends Digest::Base for some
reason.
[1] ruby/ruby#3149