Project

General

Profile

Actions

Patch #35217

closed

Replace use of Digest::MD5 / Digest::SHA1 with ActiveSupport::Digest

Added by Jens Krämer over 3 years ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Rails introduced ActiveSupport::Digest to allow central configuration of the actual digest implementation that is used throughout Rails. This is helpful in environments where certain digest implementations (most notably, MD5) are not available, i.e. to be FIPS compliant.

The attached patch replaces all uses of Digest::SHA1 and Digest::MD5 with ActiveSupport::Digest. Without further configuration, this will result in Digest::SHA1 being used in all these instances since that's the current Rails default. This can be changed by users via the config.active_support.hash_digest_class setting , i.e.:

Rails.application.config.active_support.hash_digest_class = OpenSSL::Digest::SHA256

Files


Related issues

Related to Redmine - Patch #40652: Replace MD5 with SHA256 when creating the hash for gravatar URLClosedMarius BĂLTEANU

Actions
Actions #1

Updated by Pavel Rosický over 3 years ago

thanks for working on this!

however, the OpenID change isn't safe. The SHA1 algorithm is hardcoded here and your change will break it.
https://github.com/redmine/redmine/blob/49e323ae7af2998fc2785319643a9ac5bc93c425/lib/plugins/open_id_authentication/test/mem_cache_store_test.rb#L126

https://github.com/openid/ruby-openid do support SHA256, maybe add an option to choose it? It has to be a separate option, it can't depend on Rails.application.config.active_support.hash_digest_class

the second missing part is gravatars https://github.com/redmine/redmine/blob/master/lib/plugins/gravatar/lib/gravatar.rb#L68
as discussed in https://www.redmine.org/boards/2/topics/65253 I don't think there's a way to support this feature without MD5, so if the digest isn't available, the feature has to be disabled.

Actions #2

Updated by Marius BĂLTEANU 6 months ago

Pavel Rosický wrote in #note-1:

thanks for working on this!

however, the OpenID change isn't safe. The SHA1 algorithm is hardcoded here and your change will break it.
https://github.com/redmine/redmine/blob/49e323ae7af2998fc2785319643a9ac5bc93c425/lib/plugins/open_id_authentication/test/mem_cache_store_test.rb#L126

https://github.com/openid/ruby-openid do support SHA256, maybe add an option to choose it? It has to be a separate option, it can't depend on Rails.application.config.active_support.hash_digest_class

The openid plugin was removed so these issues are no longer available.

the second missing part is gravatars https://github.com/redmine/redmine/blob/master/lib/plugins/gravatar/lib/gravatar.rb#L68
as discussed in https://www.redmine.org/boards/2/topics/65253 I don't think there's a way to support this feature without MD5, so if the digest isn't available, the feature has to be disabled.

It seems that gravatar supports now also SHA256: https://docs.gravatar.com/avatars/hash/.

Should we take this change into consideration for 6.0.0? Jens, do you have any updated patch?

Actions #3

Updated by Jens Krämer 6 months ago

I would suggest to just swap out MD5 for SHA256 for the Gravatar use case. It seems not practical to tie this to the hash_digest_class configuration, so let's just hard code the algorithm suggested by the Gravatar docs here.

Actions #4

Updated by Marius BĂLTEANU 6 months ago

Jens Krämer wrote in #note-3:

I would suggest to just swap out MD5 for SHA256 for the Gravatar use case. It seems not practical to tie this to the hash_digest_class configuration, so let's just hard code the algorithm suggested by the Gravatar docs here.

Agree, I've posted the patch in #40652.

Actions #5

Updated by Pavel Rosický 6 months ago

yeah, it looks like https://www.gravatar.com does support SHA256 now, it wasn't supported at the time when I wrote the comment (3 years ago)

no other objections then :)

Actions #6

Updated by Marius BĂLTEANU 6 months ago

  • Related to Patch #40652: Replace MD5 with SHA256 when creating the hash for gravatar URL added
Actions #7

Updated by Marius BĂLTEANU 6 months ago

  • Assignee set to Marius BĂLTEANU
  • Target version set to 6.0.0
Actions #8

Updated by Marius BĂLTEANU 6 months ago

  • Category set to Code cleanup/refactoring
  • Status changed from New to Closed

Patch committed, thanks Jens, Pavel!

Actions

Also available in: Atom PDF