Change Content MD5 to X-Authorization-Content-SHA256

fwininger commented 4 years ago

related to #176

fwininger commented 4 years ago

@mgomes can you review this please ?

mgomes commented 4 years ago

@fwininger this is looking good. We'll probably need to cut a version 3.0 version of the gem when this lands.

I was looking through the AWS docs to see if there is a convention for the header name but I couldn't find any. Just curious, what made you go with X-Authorization-Content-SHA256?

fwininger commented 4 years ago

@mgomes for X-Authorization-Content-SHA256 you ! See https://github.com/mgomes/api_auth/issues/176#issuecomment-525309683

AWS use x-amz-content-sha256

https://docs.aws.amazon.com/AmazonS3/latest/API/bucket-policy-s3-sigv4-conditions.html

mgomes commented 4 years ago

🤦 sorry about that.

fwininger commented 4 years ago

@fwininger this is looking good. We'll probably need to cut a version 3.0 version of the gem when this lands.

I was looking through the AWS docs to see if there is a convention for the header name but I couldn't find any. Just curious, what made you go with X-Authorization-Content-SHA256?

I'm agree with the version 3.0. Before we release the next major version, I would like to implement some change to be more 2020 compliant. When this MR is correct, I change the Readme to use SHA-256 by default, with introduction to be backward compatible with SHA-1. I update all reference, for example FIPS-180...

fwininger commented 4 years ago

I would like also implement a option to force X-Authorization-Content-SHA256 when the body length is not 0.

fwininger commented 4 years ago

@mgomes have you time to review this PR ?

fwininger commented 4 years ago

@mgomes @kjg have you time to review this PR ? I have a regulatory audit next month and we have an offense to the previous audit with the usage of MD5.

fwininger commented 4 years ago

@mgomes Yes I would like to clean the Readme, but I prefer to do this after this PR is merged.

I will appreciate to become a member of this repo, I can manage the PR review and fix bugs, but I will appreciate if you can continue to manage the release part.

tycooon commented 3 years ago

It looks like this update is not documented in the CHANGELOG and is not backwards compatible. @mgomes maybe it's time to bump the major version to 3.0 and maybe also add some docs for upgrade process?

CamilleDrapier commented 3 years ago

Thanks for implementing more robust security.

On top of having a changelog entry, it would have been nice to have a least one version that supports both md5 (with a deprecation message logged?) and sha-256, to make clients transition easier. :bow:

rapito commented 3 years ago

Kudos for increasing the security here!

It looks like this update is not documented in the CHANGELOG and is not backwards compatible. @mgomes maybe it's time to bump the major version to 3.0 and maybe also add some docs for upgrade process?

I came here because of this. SemVer updated to the next non major version and things started breaking... Also, this not only affects people using this gem but anyone who consumes our API. :(

Thanks for implementing more robust security.

On top of having a changelog entry, it would have been nice to have a least one version that supports both md5 (with a deprecation message logged?) and sha-256, to make clients transition easier. 🙇

I agree with @CamilleDrapier here, is there any way a newer version can be published with backwards compatibility and move this change up to 3.0?

Also, @fwininger would it be possible to document these changes on the changelog as part of this PR (even if commited retroactively, since this is already merged in and released)?

CamilleDrapier commented 3 years ago

By the way, I have ended up writing a monkeypatch for handling both of these (assuming a Rails application), it comes of course without guarantee of work nor support, nor anything :innocent: :

initializer:

require lib.join "extensions", "api_auth", "helpers.rb"
require lib.join "extensions", "api_auth", "railtie.rb"
require lib.join "extensions", "api_auth", "request_drivers", "action_controller.rb"

lib/extensions/api_auth/helpers.rb:

require "api-auth"

# Monkey patch the ApiAuth module to support the legacy MD5 hash header
module ApiAuth
 # MONKEYPATCH: restore function that was removed in https://github.com/mgomes/api_auth/pull/188/files?file-filters%5B%5D=.rb
 module Helpers
   def md5_base64digest(string)
     Digest::MD5.base64digest(string)
   end
 end
end

lib/extensions/api_auth/railtie.rb:

require "api-auth"

# Monkey patch the ApiAuth module to support the legacy MD5 hash header
module ApiAuth
 # Integration with Rails
 #
 class Rails # :nodoc:
   module ActiveResourceExtension # :nodoc:
     module Connection
       def request_with_auth(method, path, *arguments)
         if use_hmac && hmac_access_id && hmac_secret_key
           h = arguments.last
           tmp = "Net::HTTP::#{method.to_s.capitalize}".constantize.new(path, h)
           tmp.body = arguments[0] if arguments.length > 1
           ApiAuth.sign!(tmp, hmac_access_id, hmac_secret_key, api_auth_options)
           # MONKEYPATCH BEGIN
           arguments.last['Content-MD5'] = tmp['Content-MD5'] if tmp['Content-MD5']
           # MONKEYPATCH END
           if tmp['X-Authorization-Content-SHA256']
             arguments.last['X-Authorization-Content-SHA256'] = tmp['X-Authorization-Content-SHA256']
           end
           arguments.last['DATE'] = tmp['DATE']
           arguments.last['Authorization'] = tmp['Authorization']
         end
         request_without_auth(method, path, *arguments)
       end
     end # Connection
   end
 end
end

lib/extensions/api_auth/request_drivers/action_controller.rb:

require "api-auth"

# Monkey patch the ApiAuth module to support the legacy MD5 hash header
module ApiAuth
 module RequestDrivers # :nodoc:
   class ActionControllerRequest # :nodoc:
     # MONKEYPATCH: restore function that was removed in https://github.com/mgomes/api_auth/compare/2.4.1...2.5.0#diff-5e699468b10febb2c061cbcbb3fe8af211563938635ac946dc73f9ed6a0e254bL18
     def calculated_md5
       body = @request.raw_post
       md5_base64digest(body)
     end

     # MONKEYPATCH: populate MD5, see https://github.com/mgomes/api_auth/compare/2.4.1...2.5.0#diff-5e699468b10febb2c061cbcbb3fe8af211563938635ac946dc73f9ed6a0e254bL23-R28
     def populate_content_hash
       return unless @request.put? || @request.post?

       # MONKEYPATCH BEGIN
       @request.env["Content-MD5"] = calculated_md5
       # MONKEYPATCH END
       @request.env["X-AUTHORIZATION-CONTENT-SHA256"] = calculated_hash
       fetch_headers
     end

     # MONKEYPATCH: allow MD5 match, see https://github.com/mgomes/api_auth/compare/2.4.1...2.5.0#diff-5e699468b10febb2c061cbcbb3fe8af211563938635ac946dc73f9ed6a0e254bL30-R36
     def content_hash_mismatch?
       if @request.put? || @request.post?
         # MONKEYPATCH BEGIN
         calculated_hash != content_hash && calculated_md5 != content_hash
         # MONKEYPATCH END
       else
         false
       end
     end

     # MONKEYPATCH: Allow both SHA256 and legacy MD5, see https://github.com/mgomes/api_auth/compare/2.4.1...2.5.0#diff-5e699468b10febb2c061cbcbb3fe8af211563938635ac946dc73f9ed6a0e254bL50-R51
     def content_hash
       find_header(%w[X-AUTHORIZATION-CONTENT-SHA256 CONTENT-MD5 CONTENT_MD5 HTTP_CONTENT_MD5])
     end
   end
 end
end

mgomes / api_auth

Change Content MD5 to X-Authorization-Content-SHA256 #188