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

Initial support for running Rails on FIPS-certified systems #31289

Merged
merged 1 commit into from Dec 14, 2017

Conversation

Projects
None yet
@dmitri-d
Contributor

dmitri-d commented Nov 30, 2017

This PR introduces initial support for running Rails on system in FIPS mode. These systems do not have access to commonly used hash functions and MD5 in particular. Attempts to calculate MD5 hashes using openssl on a linux system will result in termination of the Rails process.

This PR allows to switch from MD5 hashes to SHA512 hashes by setting config.active_support.use_fips_approved_hash_function setting to true.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Nov 30, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Nov 30, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Nov 30, 2017

Contributor

Why SHA512 if SHA256 is also approved and considered secure? Speed and output sizes (urlsafe Base64 instead of hex?) are valid concerns here.

Heck, FIPS 140-2 Annex A still lists SHA1 and is nowadays faster than MD5 due to optimizations. From my 2016 Macbook with OpenSSL from Homebrew:

OpenSSL 1.0.2l  25 May 2017
built on: reproducible build, date unspecified
options:bn(64,64) rc4(ptr,int) des(idx,cisc,16,int) aes(partial) idea(int) blowfish(idx)
compiler: clang -I. -I.. -I../include  -fPIC -fno-common -DOPENSSL_PIC -DZLIB_SHARED -DZLIB -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -arch x86_64 -O3 -DL_ENDIAN -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM -DECP_NISTZ256_ASM
The 'numbers' are in 1000s of bytes per second processed.
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5              63653.10k   189256.22k   417463.37k   613223.08k   699402.92k
sha1             72633.27k   221723.41k   524869.80k   813962.92k   975909.42k
Contributor

bdewater commented Nov 30, 2017

Why SHA512 if SHA256 is also approved and considered secure? Speed and output sizes (urlsafe Base64 instead of hex?) are valid concerns here.

Heck, FIPS 140-2 Annex A still lists SHA1 and is nowadays faster than MD5 due to optimizations. From my 2016 Macbook with OpenSSL from Homebrew:

OpenSSL 1.0.2l  25 May 2017
built on: reproducible build, date unspecified
options:bn(64,64) rc4(ptr,int) des(idx,cisc,16,int) aes(partial) idea(int) blowfish(idx)
compiler: clang -I. -I.. -I../include  -fPIC -fno-common -DOPENSSL_PIC -DZLIB_SHARED -DZLIB -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -arch x86_64 -O3 -DL_ENDIAN -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM -DECP_NISTZ256_ASM
The 'numbers' are in 1000s of bytes per second processed.
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5              63653.10k   189256.22k   417463.37k   613223.08k   699402.92k
sha1             72633.27k   221723.41k   524869.80k   813962.92k   975909.42k
@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Nov 30, 2017

Contributor

@bdewater: You are indeed correct, SHA-1 is fips-approved. Should've looked at the Annex A instead of relying on Oracle's list of unapproved functions (https://docs.oracle.com/cd/E36784_01/html/E54953/fips-notok-1.html), which confused me. I replaced SHA512 with SHA1.

Contributor

dmitri-d commented Nov 30, 2017

@bdewater: You are indeed correct, SHA-1 is fips-approved. Should've looked at the Annex A instead of relying on Oracle's list of unapproved functions (https://docs.oracle.com/cd/E36784_01/html/E54953/fips-notok-1.html), which confused me. I replaced SHA512 with SHA1.

Show outdated Hide outdated activesupport/lib/active_support/digest.rb Outdated
@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Nov 30, 2017

Contributor

Replaced constructor argument with hash.

Contributor

dmitri-d commented Nov 30, 2017

Replaced constructor argument with hash.

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Nov 30, 2017

Contributor

I had a thought I forgot to type in my original comment; why not switch to SHA1 everywhere instead of providing a configuration option? While we're making changes, we can even shorten the hash output compared to what it was. E.g. for etags:

Digest::MD5.hexdigest('a')
=> "0cc175b9c0f1b6a831c399e269772661"

Base64.urlsafe_encode64(Digest::SHA1.digest('a'))
=> "hvfkN_qlp_zhXR3cuerq6jd2Z7g="

Seems like a win-win?

Contributor

bdewater commented Nov 30, 2017

I had a thought I forgot to type in my original comment; why not switch to SHA1 everywhere instead of providing a configuration option? While we're making changes, we can even shorten the hash output compared to what it was. E.g. for etags:

Digest::MD5.hexdigest('a')
=> "0cc175b9c0f1b6a831c399e269772661"

Base64.urlsafe_encode64(Digest::SHA1.digest('a'))
=> "hvfkN_qlp_zhXR3cuerq6jd2Z7g="

Seems like a win-win?

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Nov 30, 2017

Contributor

why not switch to SHA1 everywhere instead of providing a configuration option?

My reasoning for the approach I chose was that not everyone might want to switch away from MD5. Using just SHA1 would be ok with me though. I also think that keeping the abstraction would be useful, if only because FIPS will eventually drop SHA1 from the list of approved functions.

Contributor

dmitri-d commented Nov 30, 2017

why not switch to SHA1 everywhere instead of providing a configuration option?

My reasoning for the approach I chose was that not everyone might want to switch away from MD5. Using just SHA1 would be ok with me though. I also think that keeping the abstraction would be useful, if only because FIPS will eventually drop SHA1 from the list of approved functions.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Nov 30, 2017

Contributor

@bdewater: to clarify -- I can go ahead with the change we discussed?

Contributor

dmitri-d commented Nov 30, 2017

@bdewater: to clarify -- I can go ahead with the change we discussed?

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
@georgeclaghorn

georgeclaghorn Dec 1, 2017

Member

I had a thought I forgot to type in my original comment; why not switch to SHA1 everywhere instead of providing a configuration option?

For one thing, changing the hashing algorithm used to generate cache keys—effectively invalidating every cache entry—is particularly objectionable for existing apps.

Member

georgeclaghorn commented Dec 1, 2017

I had a thought I forgot to type in my original comment; why not switch to SHA1 everywhere instead of providing a configuration option?

For one thing, changing the hashing algorithm used to generate cache keys—effectively invalidating every cache entry—is particularly objectionable for existing apps.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Dec 1, 2017

Member

Caches surviving a Rails release bump have been pretty hit-and-miss historically. I don't think that's an automatic deal-breaker.

Arguably, we about as likely to be mis-caching something that's supposed to have changed [due to some change in Rails methods it's calling] as we are to be missing a cache that would've returned the same value.

Member

matthewd commented Dec 1, 2017

Caches surviving a Rails release bump have been pretty hit-and-miss historically. I don't think that's an automatic deal-breaker.

Arguably, we about as likely to be mis-caching something that's supposed to have changed [due to some change in Rails methods it's calling] as we are to be missing a cache that would've returned the same value.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 1, 2017

Contributor

Ok, how should we proceed? Keep hash algorithm as a setting? Perhaps rename setting to sound more generic (like "use_strong_hashing")? Or drop it altogether and switch to SHA1?

Contributor

dmitri-d commented Dec 1, 2017

Ok, how should we proceed? Keep hash algorithm as a setting? Perhaps rename setting to sound more generic (like "use_strong_hashing")? Or drop it altogether and switch to SHA1?

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Dec 1, 2017

Member

I would like to rename the setting to something more generic, probably there are many users that would like to adopt this but not because of FIPS certification.

Member

guilleiguaran commented Dec 1, 2017

I would like to rename the setting to something more generic, probably there are many users that would like to adopt this but not because of FIPS certification.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Dec 2, 2017

Member

I think it's hard to apply a single "strong hashing" setting, and this patch wouldn't do it: you're currently touching the places that use MD5, which are places where (in our judgement) it doesn't matter -- we're using it as a wider CRC32, not a security mechanism.

Offering to use SHA512, or some future super-secure hash function, to calculate totally internal cache digests seems pretty pointless while we're using SHA256 at best in places where it's actually important.

I'm tempted to rename AS::Digest to something like AS::InsecureDigest, and hard-code it to SHA1. Then when a future FIPS also pointlessly insists that's terrible, people will be able to monkey-patch that single method to get things working -- and we'll probably change it to use the first 30-40 nibbles of SHA256.

But I'd like to hear some more opinions on @georgeclaghorn's concern about the mass cache invalidation first. @jeremy @rafaelfranca?

Member

matthewd commented Dec 2, 2017

I think it's hard to apply a single "strong hashing" setting, and this patch wouldn't do it: you're currently touching the places that use MD5, which are places where (in our judgement) it doesn't matter -- we're using it as a wider CRC32, not a security mechanism.

Offering to use SHA512, or some future super-secure hash function, to calculate totally internal cache digests seems pretty pointless while we're using SHA256 at best in places where it's actually important.

I'm tempted to rename AS::Digest to something like AS::InsecureDigest, and hard-code it to SHA1. Then when a future FIPS also pointlessly insists that's terrible, people will be able to monkey-patch that single method to get things working -- and we'll probably change it to use the first 30-40 nibbles of SHA256.

But I'd like to hear some more opinions on @georgeclaghorn's concern about the mass cache invalidation first. @jeremy @rafaelfranca?

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 2, 2017

Contributor

I think it's hard to apply a single "strong hashing" setting, and this patch wouldn't do it: you're currently touching the places that use MD5, which are places where (in our judgement) it doesn't matter -- we're using it as a wider CRC32, not a security mechanism.

I agree that MD5 is perfectly adequate for generating of cache keys, unfortunately FIPS-certified OpenSSL environment is heavy-handed in its approach, and MD5 isn't available at all.

I'm tempted to rename AS::Digest to something like AS::InsecureDigest, and hard-code it to SHA1. Then when a future FIPS also pointlessly insists that's terrible, people will be able to monkey-patch that single method to get things working -- and we'll probably change it to use the first 30-40 nibbles of SHA256.

Sounds good re: renaming; hardcoding SHA1 would be my preference too.

Contributor

dmitri-d commented Dec 2, 2017

I think it's hard to apply a single "strong hashing" setting, and this patch wouldn't do it: you're currently touching the places that use MD5, which are places where (in our judgement) it doesn't matter -- we're using it as a wider CRC32, not a security mechanism.

I agree that MD5 is perfectly adequate for generating of cache keys, unfortunately FIPS-certified OpenSSL environment is heavy-handed in its approach, and MD5 isn't available at all.

I'm tempted to rename AS::Digest to something like AS::InsecureDigest, and hard-code it to SHA1. Then when a future FIPS also pointlessly insists that's terrible, people will be able to monkey-patch that single method to get things working -- and we'll probably change it to use the first 30-40 nibbles of SHA256.

Sounds good re: renaming; hardcoding SHA1 would be my preference too.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 5, 2017

Contributor

@georgeclaghorn, @jeremy @rafaelfranca -- thoughts/comments?

Contributor

dmitri-d commented Dec 5, 2017

@georgeclaghorn, @jeremy @rafaelfranca -- thoughts/comments?

Show outdated Hide outdated activesupport/test/digest_test.rb Outdated
@simi

This comment has been minimized.

Show comment
Hide comment
@simi

simi Dec 5, 2017

Contributor

What about to let user pass custom object responding to hexdigest(arg) in configuration and make ::Digest::MD5 default one instead of selecting between those two (MD5/SHA1)? We can skip all those fips naming then and introduce generic configuration where you can use both MD5/SHA1 from stdlib easily or introduce your own concept of "hexdigesting".

edit: That config can be used easily in the future to migrate apps to new default if it will be decided to move to SHA1.

Contributor

simi commented Dec 5, 2017

What about to let user pass custom object responding to hexdigest(arg) in configuration and make ::Digest::MD5 default one instead of selecting between those two (MD5/SHA1)? We can skip all those fips naming then and introduce generic configuration where you can use both MD5/SHA1 from stdlib easily or introduce your own concept of "hexdigesting".

edit: That config can be used easily in the future to migrate apps to new default if it will be decided to move to SHA1.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 5, 2017

Contributor

What about to let user pass custom object responding to hexdigest(arg) in configuration

Why would such a level of flexibility be useful? How realistic is that someone would want to use a hash function other than SHA1 or MD5 (esp. considering that the sole driver for this change is FIPS compatibility). If anything, such an approach could lead to some "interesting" runtime errors.

Contributor

dmitri-d commented Dec 5, 2017

What about to let user pass custom object responding to hexdigest(arg) in configuration

Why would such a level of flexibility be useful? How realistic is that someone would want to use a hash function other than SHA1 or MD5 (esp. considering that the sole driver for this change is FIPS compatibility). If anything, such an approach could lead to some "interesting" runtime errors.

@simi

This comment has been minimized.

Show comment
Hide comment
@simi

simi Dec 5, 2017

Contributor

Why would such a level of flexibility be useful? How realistic is that someone would want to use a hash function other than SHA1 or MD5 (esp. considering that the sole driver for this change is FIPS compatibility). If anything, such an approach could lead to some "interesting" runtime errors.

The code will get simpler, configuration will get more transparent and it will add ability to bring in custom hexdigests algorithm. In FIPS env. you'll be able to switch to SHA1 without actually letting Rails know anything about FIPS and Rails core can decide to move to SHA1 and prepare standard way how to upgrade existing apps (via initializer config).

Contributor

simi commented Dec 5, 2017

Why would such a level of flexibility be useful? How realistic is that someone would want to use a hash function other than SHA1 or MD5 (esp. considering that the sole driver for this change is FIPS compatibility). If anything, such an approach could lead to some "interesting" runtime errors.

The code will get simpler, configuration will get more transparent and it will add ability to bring in custom hexdigests algorithm. In FIPS env. you'll be able to switch to SHA1 without actually letting Rails know anything about FIPS and Rails core can decide to move to SHA1 and prepare standard way how to upgrade existing apps (via initializer config).

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Dec 5, 2017

Contributor

That's a valid concern too, for example in https://github.com/Shopify/identity_cache/blob/master/lib/identity_cache/cache_hash.rb we prefer CityHash when available for performance reasons.

Contributor

bdewater commented Dec 5, 2017

That's a valid concern too, for example in https://github.com/Shopify/identity_cache/blob/master/lib/identity_cache/cache_hash.rb we prefer CityHash when available for performance reasons.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 7, 2017

Contributor
  • ActiveSupport::Digest can now be configured with a custom digest class. Uses Digest::MD5 as default implementation.
Contributor

dmitri-d commented Dec 7, 2017

  • ActiveSupport::Digest can now be configured with a custom digest class. Uses Digest::MD5 as default implementation.
Show outdated Hide outdated activesupport/lib/active_support/digest.rb Outdated
@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 8, 2017

Contributor
  • Replaced conditional with logical expression
Contributor

dmitri-d commented Dec 8, 2017

  • Replaced conditional with logical expression
Show outdated Hide outdated activesupport/lib/active_support/digest.rb Outdated
@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 8, 2017

Contributor
  • Renamed 'clazz' parameter to 'klass'
Contributor

dmitri-d commented Dec 8, 2017

  • Renamed 'clazz' parameter to 'klass'
@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 11, 2017

Contributor

Any other comments? Can this be merged now?

Contributor

dmitri-d commented Dec 11, 2017

Any other comments? Can this be merged now?

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Dec 11, 2017

Member

@witlessbird can you add an entry to the top of the action pack changelog and squash your commits?

Member

eileencodes commented Dec 11, 2017

@witlessbird can you add an entry to the top of the action pack changelog and squash your commits?

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 11, 2017

Contributor
  • updated CHANGELOG
  • squashed commits
Contributor

dmitri-d commented Dec 11, 2017

  • updated CHANGELOG
  • squashed commits
@@ -183,7 +182,7 @@ def normalize_key(key, options)
key = super.dup
key = key.force_encoding(Encoding::ASCII_8BIT)
key = key.gsub(ESCAPE_KEY_CHARS) { |match| "%#{match.getbyte(0).to_s(16).upcase}" }
key = "#{key[0, 213]}:md5:#{Digest::MD5.hexdigest(key)}" if key.size > 250
key = "#{key[0, 213]}:md5:#{ActiveSupport::Digest.hexdigest(key)}" if key.size > 250

This comment has been minimized.

@bdewater

bdewater Dec 11, 2017

Contributor

Won't this need to be dynamic now the hash is not hardcoded anymore? ":md5:#{Digest::MD5.hexdigest(key)}" is 37 characters, but with SHA1 it's 45 so it'll go over the 250 byte limit.

@bdewater

bdewater Dec 11, 2017

Contributor

Won't this need to be dynamic now the hash is not hardcoded anymore? ":md5:#{Digest::MD5.hexdigest(key)}" is 37 characters, but with SHA1 it's 45 so it'll go over the 250 byte limit.

This comment has been minimized.

@dmitri-d

dmitri-d Dec 11, 2017

Contributor

I saw that, but didn't think it was necessary to change. My understanding is that this is only a hint in the key, and doesn't effect the functionality in any way. TBH, I'm not sure why use md5 (or other labels) there at all.

@dmitri-d

dmitri-d Dec 11, 2017

Contributor

I saw that, but didn't think it was necessary to change. My understanding is that this is only a hint in the key, and doesn't effect the functionality in any way. TBH, I'm not sure why use md5 (or other labels) there at all.

This comment has been minimized.

@eugeneius

eugeneius Dec 11, 2017

Member

Memcached keys can't be longer than 250 characters:

https://github.com/memcached/memcached/blob/master/doc/protocol.txt

Data stored by memcached is identified with the help of a key. A key is a text string which should uniquely identify the data for clients that are interested in storing and retrieving it. Currently the length limit of a key is set at 250 characters (of course, normally clients wouldn't need to use such long keys); the key must not include control characters or whitespace.

We should truncate the output of ActiveSupport::Digest.hexdigest to 32 characters.

@eugeneius

eugeneius Dec 11, 2017

Member

Memcached keys can't be longer than 250 characters:

https://github.com/memcached/memcached/blob/master/doc/protocol.txt

Data stored by memcached is identified with the help of a key. A key is a text string which should uniquely identify the data for clients that are interested in storing and retrieving it. Currently the length limit of a key is set at 250 characters (of course, normally clients wouldn't need to use such long keys); the key must not include control characters or whitespace.

We should truncate the output of ActiveSupport::Digest.hexdigest to 32 characters.

Show outdated Hide outdated activesupport/test/digest_test.rb Outdated
Show outdated Hide outdated activesupport/lib/active_support/digest.rb Outdated
Show outdated Hide outdated activesupport/CHANGELOG.md Outdated
@@ -183,7 +182,7 @@ def normalize_key(key, options)
key = super.dup
key = key.force_encoding(Encoding::ASCII_8BIT)
key = key.gsub(ESCAPE_KEY_CHARS) { |match| "%#{match.getbyte(0).to_s(16).upcase}" }
key = "#{key[0, 213]}:md5:#{Digest::MD5.hexdigest(key)}" if key.size > 250
key = "#{key[0, 213]}:md5:#{ActiveSupport::Digest.hexdigest(key)}" if key.size > 250

This comment has been minimized.

@eugeneius

eugeneius Dec 11, 2017

Member

Memcached keys can't be longer than 250 characters:

https://github.com/memcached/memcached/blob/master/doc/protocol.txt

Data stored by memcached is identified with the help of a key. A key is a text string which should uniquely identify the data for clients that are interested in storing and retrieving it. Currently the length limit of a key is set at 250 characters (of course, normally clients wouldn't need to use such long keys); the key must not include control characters or whitespace.

We should truncate the output of ActiveSupport::Digest.hexdigest to 32 characters.

@eugeneius

eugeneius Dec 11, 2017

Member

Memcached keys can't be longer than 250 characters:

https://github.com/memcached/memcached/blob/master/doc/protocol.txt

Data stored by memcached is identified with the help of a key. A key is a text string which should uniquely identify the data for clients that are interested in storing and retrieving it. Currently the length limit of a key is set at 250 characters (of course, normally clients wouldn't need to use such long keys); the key must not include control characters or whitespace.

We should truncate the output of ActiveSupport::Digest.hexdigest to 32 characters.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 11, 2017

Contributor
  • fixed a spelling error in CHANGELOG
  • updated test to use hash_digest_class setting in custom hash function test
  • Truncated digest used to generate memcached key to 32 characters
Contributor

dmitri-d commented Dec 11, 2017

  • fixed a spelling error in CHANGELOG
  • updated test to use hash_digest_class setting in custom hash function test
  • Truncated digest used to generate memcached key to 32 characters
@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Dec 12, 2017

Member

I still think we should just change it to SHA1. Exposing a setting just in case someone wants it feels very un-Rails.

Member

matthewd commented Dec 12, 2017

I still think we should just change it to SHA1. Exposing a setting just in case someone wants it feels very un-Rails.

@@ -133,7 +133,7 @@ def generate_weak_etag(validators)
end
def generate_strong_etag(validators)
%("#{Digest::MD5.hexdigest(ActiveSupport::Cache.expand_cache_key(validators))}")
%("#{ActiveSupport::Digest.hexdigest(ActiveSupport::Cache.expand_cache_key(validators))}")

This comment has been minimized.

@eugeneius

eugeneius Dec 12, 2017

Member

I'm not sure it's a good idea for the length of the ETag header to reveal which digest algorithm is configured.

When I suggested truncating the output of hexdigest, I meant inside our wrapper function; we shouldn't burden callers with deciding whether they need a fixed length string.

Of course, switching to SHA-1 everywhere as @matthewd suggested would also resolve this issue.

@eugeneius

eugeneius Dec 12, 2017

Member

I'm not sure it's a good idea for the length of the ETag header to reveal which digest algorithm is configured.

When I suggested truncating the output of hexdigest, I meant inside our wrapper function; we shouldn't burden callers with deciding whether they need a fixed length string.

Of course, switching to SHA-1 everywhere as @matthewd suggested would also resolve this issue.

Introduced `ActiveSupport::Digest` that allows to specify hash functi…
…on implementation

and defaults to `Digest::MD5`.

Replaced calls to `::Digest::MD5.hexdigest` with calls to `ActiveSupport::Digest.hexdigest`.
@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 12, 2017

Contributor
  • truncated ActiveSupport::Digest#hexdigest output to 32 characters.
Contributor

dmitri-d commented Dec 12, 2017

  • truncated ActiveSupport::Digest#hexdigest output to 32 characters.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 12, 2017

Member

I still think we should just change it to SHA1

👍 for me. Some caches survives Rails upgrades but we never guarantee that.

Member

rafaelfranca commented Dec 12, 2017

I still think we should just change it to SHA1

👍 for me. Some caches survives Rails upgrades but we never guarantee that.

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 14, 2017

Contributor

Does anyone object to this implementation? Could this be merged now?

Contributor

dmitri-d commented Dec 14, 2017

Does anyone object to this implementation? Could this be merged now?

@eileencodes eileencodes merged commit 659c516 into rails:master Dec 14, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Dec 14, 2017

Member

Thanks @witlessbird! And congrats on your first Rails contribution 🎉

Member

eileencodes commented Dec 14, 2017

Thanks @witlessbird! And congrats on your first Rails contribution 🎉

@dmitri-d

This comment has been minimized.

Show comment
Hide comment
@dmitri-d

dmitri-d Dec 14, 2017

Contributor

Thanks for suggestions and reviews, y'all!

Contributor

dmitri-d commented Dec 14, 2017

Thanks for suggestions and reviews, y'all!

@simi

This comment has been minimized.

Show comment
Hide comment
@simi

simi Dec 14, 2017

Contributor

xjyemei

Contributor

simi commented Dec 14, 2017

xjyemei

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment