Skip to content

Commit

Permalink
Migrate all top level uses of ::Digest to OpenSSL::Digest.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
vipulnsward committed Jun 28, 2020
1 parent 35ebac1 commit 94ff09b
Show file tree
Hide file tree
Showing 33 changed files with 70 additions and 69 deletions.
Expand Up @@ -3,7 +3,7 @@
gem "pg", "~> 1.1"
require "pg"
require "thread"
require "digest/sha1"
require "openssl"

module ActionCable
module SubscriptionAdapter
Expand Down Expand Up @@ -58,7 +58,7 @@ def with_broadcast_connection(&block) # :nodoc:

private
def channel_identifier(channel)
channel.size > 63 ? Digest::SHA1.hexdigest(channel) : channel
channel.size > 63 ? OpenSSL::Digest.hexdigest("SHA1", channel) : channel
end

def listener
Expand Down
Expand Up @@ -116,7 +116,7 @@ def recent?
end

def expected_signature
OpenSSL::HMAC.hexdigest OpenSSL::Digest::SHA256.new, key, "#{timestamp}#{token}"
OpenSSL::HMAC.hexdigest OpenSSL::Digest.new("SHA256"), key, "#{timestamp}#{token}"
end
end
end
Expand Down
Expand Up @@ -75,7 +75,7 @@ def given_signature
end

def expected_signature
Base64.strict_encode64 OpenSSL::HMAC.digest(OpenSSL::Digest::SHA1.new, key, message)
Base64.strict_encode64 OpenSSL::HMAC.digest(OpenSSL::Digest.new("SHA1"), key, message)
end

def message
Expand Down
Expand Up @@ -14,7 +14,7 @@ module ActionMailbox::InboundEmail::MessageId
# attachment called +raw_email+. Before the upload, extract the Message-ID from the +source+ and set
# it as an attribute on the new +InboundEmail+.
def create_and_extract_message_id!(source, **options)
message_checksum = Digest::SHA1.hexdigest(source)
message_checksum = OpenSSL::Digest.hexdigest("SHA1", source)
message_id = extract_message_id(source) || generate_missing_message_id(message_checksum)

create! options.merge(message_id: message_id, message_checksum: message_checksum) do |inbound_email|
Expand Down
10 changes: 5 additions & 5 deletions actionpack/lib/action_controller/metal/http_authentication.rb
Expand Up @@ -228,12 +228,12 @@ def validate_digest_response(request, realm, &password_procedure)
# of a plain-text password.
def expected_response(http_method, uri, credentials, password, password_is_ha1 = true)
ha1 = password_is_ha1 ? password : ha1(credentials, password)
ha2 = ::Digest::MD5.hexdigest([http_method.to_s.upcase, uri].join(":"))
::Digest::MD5.hexdigest([ha1, credentials[:nonce], credentials[:nc], credentials[:cnonce], credentials[:qop], ha2].join(":"))
ha2 = OpenSSL::Digest.hexdigest("MD5", [http_method.to_s.upcase, uri].join(":"))
OpenSSL::Digest.hexdigest("MD5", [ha1, credentials[:nonce], credentials[:nc], credentials[:cnonce], credentials[:qop], ha2].join(":"))
end

def ha1(credentials, password)
::Digest::MD5.hexdigest([credentials[:username], credentials[:realm], password].join(":"))
OpenSSL::Digest.hexdigest("MD5", [credentials[:username], credentials[:realm], password].join(":"))
end

def encode_credentials(http_method, credentials, password, password_is_ha1)
Expand Down Expand Up @@ -307,7 +307,7 @@ def secret_token(request)
def nonce(secret_key, time = Time.now)
t = time.to_i
hashed = [t, secret_key]
digest = ::Digest::MD5.hexdigest(hashed.join(":"))
digest = OpenSSL::Digest.hexdigest("MD5", hashed.join(":"))
::Base64.strict_encode64("#{t}:#{digest}")
end

Expand All @@ -324,7 +324,7 @@ def validate_nonce(secret_key, request, value, seconds_to_timeout = 5 * 60)

# Opaque based on digest of secret key
def opaque(secret_key)
::Digest::MD5.hexdigest(secret_key)
OpenSSL::Digest.hexdigest("MD5", secret_key)
end
end

Expand Down
Expand Up @@ -414,7 +414,7 @@ def global_csrf_token(session) # :doc:

def csrf_token_hmac(session, identifier) # :doc:
OpenSSL::HMAC.digest(
OpenSSL::Digest::SHA256.new,
OpenSSL::Digest.new("SHA256"),
real_csrf_token(session),
identifier
)
Expand Down
4 changes: 2 additions & 2 deletions actionpack/test/controller/http_digest_authentication_test.rb
Expand Up @@ -9,7 +9,7 @@ class DummyDigestController < ActionController::Base
before_action :authenticate_with_request, only: :display

USERS = { "lifo" => "world", "pretty" => "please",
"dhh" => ::Digest::MD5.hexdigest(["dhh", "SuperSecret", "secret"].join(":")) }
"dhh" => OpenSSL::Digest.hexdigest("MD5", ["dhh", "SuperSecret", "secret"].join(":")) }

def index
render plain: "Hello Secret"
Expand Down Expand Up @@ -185,7 +185,7 @@ def authenticate_with_request
test "authentication request with password stored as ha1 digest hash" do
@request.env["HTTP_AUTHORIZATION"] = encode_credentials(
username: "dhh",
password: ::Digest::MD5.hexdigest(["dhh", "SuperSecret", "secret"].join(":")),
password: OpenSSL::Digest.hexdigest("MD5", ["dhh", "SuperSecret", "secret"].join(":")),
password_is_ha1: true)
get :display

Expand Down
2 changes: 1 addition & 1 deletion actiontext/lib/action_text/attachments/caching.rb
Expand Up @@ -9,7 +9,7 @@ def cache_key(*args)

private
def cache_digest
Digest::SHA256.hexdigest(node.to_s)
OpenSSL::Digest.hexdigest("SHA256", node.to_s)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion activejob/test/support/integration/adapters/sneakers.rb
Expand Up @@ -31,7 +31,7 @@ def start_workers
@pid = fork do
queues = %w(integration_tests)
workers = queues.map do |q|
worker_klass = "ActiveJobWorker" + Digest::MD5.hexdigest(q)
worker_klass = "ActiveJobWorker" + OpenSSL::Digest.hexdigest("MD5", q)
Sneakers.const_set(worker_klass, Class.new(ActiveJob::QueueAdapters::SneakersAdapter::JobWrapper) do
from_queue q
end)
Expand Down
@@ -1,7 +1,7 @@
# frozen_string_literal: true

require "active_support/core_ext/string/access"
require "digest/sha2"
require "openssl"

module ActiveRecord
module ConnectionAdapters # :nodoc:
Expand Down Expand Up @@ -1481,7 +1481,7 @@ def strip_table_name_prefix_and_suffix(table_name)
def foreign_key_name(table_name, options)
options.fetch(:name) do
identifier = "#{table_name}_#{options.fetch(:column)}_fk"
hashed_identifier = Digest::SHA256.hexdigest(identifier).first(10)
hashed_identifier = OpenSSL::Digest.hexdigest("SHA256", identifier).first(10)

"fk_rails_#{hashed_identifier}"
end
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/tasks/database_tasks.rb
Expand Up @@ -523,7 +523,7 @@ def local_database?(db_config)
end

def schema_sha1(file)
Digest::SHA1.hexdigest(File.read(file))
OpenSSL::Digest.hexdigest("SHA1", File.read(file))
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion activestorage/app/models/active_storage/blob.rb
Expand Up @@ -281,7 +281,7 @@ def service

private
def compute_checksum_in_chunks(io)
Digest::MD5.new.tap do |checksum|
OpenSSL::Digest.new("MD5").tap do |checksum|
while chunk = io.read(5.megabytes)
checksum << chunk
end
Expand Down
2 changes: 1 addition & 1 deletion activestorage/app/models/active_storage/variant.rb
Expand Up @@ -69,7 +69,7 @@ def processed

# Returns a combination key of the blob and the variation that together identifies a specific variant.
def key
"variants/#{blob.key}/#{Digest::SHA256.hexdigest(variation.key)}"
"variants/#{blob.key}/#{OpenSSL::Digest.hexdigest("SHA256", variation.key)}"
end

# Returns the URL of the blob variant on the service. See {ActiveStorage::Blob#url} for details.
Expand Down
2 changes: 1 addition & 1 deletion activestorage/app/models/active_storage/variation.rb
Expand Up @@ -59,7 +59,7 @@ def key
end

def digest
Digest::SHA1.base64digest Marshal.dump(transformations)
OpenSSL::Digest.base64digest("SHA1", Marshal.dump(transformations))
end

private
Expand Down
2 changes: 1 addition & 1 deletion activestorage/lib/active_storage/downloader.rb
Expand Up @@ -35,7 +35,7 @@ def download(key, file)
end

def verify_integrity_of(file, checksum:)
unless Digest::MD5.file(file).base64digest == checksum
unless OpenSSL::Digest.new("MD5").file(file).base64digest == checksum
raise ActiveStorage::IntegrityError
end
end
Expand Down
4 changes: 2 additions & 2 deletions activestorage/lib/active_storage/service/disk_service.rb
Expand Up @@ -2,7 +2,7 @@

require "fileutils"
require "pathname"
require "digest/md5"
require "openssl"
require "active_support/core_ext/numeric/bytes"

module ActiveStorage
Expand Down Expand Up @@ -154,7 +154,7 @@ def make_path_for(key)
end

def ensure_integrity_of(key, checksum)
unless Digest::MD5.file(path_for(key)).base64digest == checksum
unless OpenSSL::Digest.new("MD5").file(path_for(key)).base64digest == checksum
delete key
raise ActiveStorage::IntegrityError
end
Expand Down
10 changes: 5 additions & 5 deletions activestorage/test/controllers/direct_uploads_controller_test.rb
Expand Up @@ -15,7 +15,7 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionDispatch::Integration
end

test "creating new direct upload" do
checksum = Digest::MD5.base64digest("Hello")
checksum = OpenSSL::Digest.base64digest("MD5", "Hello")

post rails_direct_uploads_url, params: { blob: {
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }
Expand Down Expand Up @@ -50,7 +50,7 @@ class ActiveStorage::GCSDirectUploadsControllerTest < ActionDispatch::Integratio
end

test "creating new direct upload" do
checksum = Digest::MD5.base64digest("Hello")
checksum = OpenSSL::Digest.base64digest("MD5", "Hello")

post rails_direct_uploads_url, params: { blob: {
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }
Expand Down Expand Up @@ -84,7 +84,7 @@ class ActiveStorage::AzureStorageDirectUploadsControllerTest < ActionDispatch::I
end

test "creating new direct upload" do
checksum = Digest::MD5.base64digest("Hello")
checksum = OpenSSL::Digest.base64digest("MD5", "Hello")

post rails_direct_uploads_url, params: { blob: {
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }
Expand All @@ -106,7 +106,7 @@ class ActiveStorage::AzureStorageDirectUploadsControllerTest < ActionDispatch::I

class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::IntegrationTest
test "creating new direct upload" do
checksum = Digest::MD5.base64digest("Hello")
checksum = OpenSSL::Digest.base64digest("MD5", "Hello")

post rails_direct_uploads_url, params: { blob: {
filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain" } }
Expand All @@ -123,7 +123,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati
end

test "creating new direct upload does not include root in json" do
checksum = Digest::MD5.base64digest("Hello")
checksum = OpenSSL::Digest.base64digest("MD5", "Hello")

set_include_root_in_json(true) do
post rails_direct_uploads_url, params: { blob: {
Expand Down
10 changes: 5 additions & 5 deletions activestorage/test/controllers/disk_controller_test.rb
Expand Up @@ -67,7 +67,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest

test "directly uploading blob with integrity" do
data = "Something else entirely!"
blob = create_blob_before_direct_upload byte_size: data.size, checksum: Digest::MD5.base64digest(data)
blob = create_blob_before_direct_upload byte_size: data.size, checksum: OpenSSL::Digest.base64digest("MD5", data)

put blob.service_url_for_direct_upload, params: data, headers: { "Content-Type" => "text/plain" }
assert_response :no_content
Expand All @@ -76,7 +76,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest

test "directly uploading blob without integrity" do
data = "Something else entirely!"
blob = create_blob_before_direct_upload byte_size: data.size, checksum: Digest::MD5.base64digest("bad data")
blob = create_blob_before_direct_upload byte_size: data.size, checksum: OpenSSL::Digest.base64digest("MD5", "bad data")

put blob.service_url_for_direct_upload, params: data
assert_response :unprocessable_entity
Expand All @@ -85,7 +85,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest

test "directly uploading blob with mismatched content type" do
data = "Something else entirely!"
blob = create_blob_before_direct_upload byte_size: data.size, checksum: Digest::MD5.base64digest(data)
blob = create_blob_before_direct_upload byte_size: data.size, checksum: OpenSSL::Digest.base64digest("MD5", data)

put blob.service_url_for_direct_upload, params: data, headers: { "Content-Type" => "application/octet-stream" }
assert_response :unprocessable_entity
Expand All @@ -95,7 +95,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
test "directly uploading blob with different but equivalent content type" do
data = "Something else entirely!"
blob = create_blob_before_direct_upload(
byte_size: data.size, checksum: Digest::MD5.base64digest(data), content_type: "application/x-gzip")
byte_size: data.size, checksum: OpenSSL::Digest.base64digest("MD5", data), content_type: "application/x-gzip")

put blob.service_url_for_direct_upload, params: data, headers: { "Content-Type" => "application/x-gzip" }
assert_response :no_content
Expand All @@ -104,7 +104,7 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest

test "directly uploading blob with mismatched content length" do
data = "Something else entirely!"
blob = create_blob_before_direct_upload byte_size: data.size - 1, checksum: Digest::MD5.base64digest(data)
blob = create_blob_before_direct_upload byte_size: data.size - 1, checksum: OpenSSL::Digest.base64digest("MD5", data)

put blob.service_url_for_direct_upload, params: data, headers: { "Content-Type" => "text/plain" }
assert_response :unprocessable_entity
Expand Down
4 changes: 2 additions & 2 deletions activestorage/test/models/blob_test.rb
Expand Up @@ -57,7 +57,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase

assert_equal data, blob.download
assert_equal data.length, blob.byte_size
assert_equal Digest::MD5.base64digest(data), blob.checksum
assert_equal OpenSSL::Digest.base64digest("MD5", data), blob.checksum
end

test "create_and_upload extracts content type from data" do
Expand Down Expand Up @@ -148,7 +148,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase

test "open without integrity" do
create_blob(data: "Hello, world!").tap do |blob|
blob.update! checksum: Digest::MD5.base64digest("Goodbye, world!")
blob.update! checksum: OpenSSL::Digest.base64digest("MD5", "Goodbye, world!")

assert_raises ActiveStorage::IntegrityError do
blob.open { |file| flunk "Expected integrity check to fail" }
Expand Down
Expand Up @@ -21,7 +21,7 @@ class ActiveStorage::Service::AzureStoragePublicServiceTest < ActiveSupport::Tes
test "direct upload" do
key = SecureRandom.base58(24)
data = "Something else entirely!"
checksum = Digest::MD5.base64digest(data)
checksum = OpenSSL::Digest.base64digest("MD5", data)
content_type = "text/xml"
url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: content_type, content_length: data.size, checksum: checksum)

Expand Down
8 changes: 4 additions & 4 deletions activestorage/test/service/azure_storage_service_test.rb
Expand Up @@ -12,7 +12,7 @@ class ActiveStorage::Service::AzureStorageServiceTest < ActiveSupport::TestCase
test "direct upload with content type" do
key = SecureRandom.base58(24)
data = "Something else entirely!"
checksum = Digest::MD5.base64digest(data)
checksum = OpenSSL::Digest.base64digest("MD5", data)
content_type = "text/xml"
url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: content_type, content_length: data.size, checksum: checksum)

Expand All @@ -34,7 +34,7 @@ class ActiveStorage::Service::AzureStorageServiceTest < ActiveSupport::TestCase
test "direct upload with content disposition" do
key = SecureRandom.base58(24)
data = "Something else entirely!"
checksum = Digest::MD5.base64digest(data)
checksum = OpenSSL::Digest.base64digest("MD5", data)
url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size, checksum: checksum)

uri = URI.parse url
Expand All @@ -56,7 +56,7 @@ class ActiveStorage::Service::AzureStorageServiceTest < ActiveSupport::TestCase
key = SecureRandom.base58(24)
data = "Foobar"

@service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
@service.upload(key, StringIO.new(data), checksum: OpenSSL::Digest.base64digest("MD5", data), filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")

url = @service.url(key, expires_in: 2.minutes, disposition: :attachment, content_type: nil, filename: ActiveStorage::Filename.new("test.html"))
response = Net::HTTP.get_response(URI(url))
Expand All @@ -70,7 +70,7 @@ class ActiveStorage::Service::AzureStorageServiceTest < ActiveSupport::TestCase
key = SecureRandom.base58(24)
data = "Foobar"

@service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), filename: ActiveStorage::Filename.new("test.txt"), disposition: :inline)
@service.upload(key, StringIO.new(data), checksum: OpenSSL::Digest.base64digest("MD5", data), filename: ActiveStorage::Filename.new("test.txt"), disposition: :inline)

assert_equal("inline; filename=\"test.txt\"; filename*=UTF-8''test.txt", @service.client.get_blob_properties(@service.container, key).properties[:content_disposition])

Expand Down
2 changes: 1 addition & 1 deletion activestorage/test/service/gcs_public_service_test.rb
Expand Up @@ -21,7 +21,7 @@ class ActiveStorage::Service::GCSPublicServiceTest < ActiveSupport::TestCase
test "direct upload" do
key = SecureRandom.base58(24)
data = "Something else entirely!"
checksum = Digest::MD5.base64digest(data)
checksum = OpenSSL::Digest.base64digest("MD5", data)
url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size, checksum: checksum)

uri = URI.parse url
Expand Down

0 comments on commit 94ff09b

Please sign in to comment.