mgomes / api_auth

HMAC authentication for Rails and HTTP Clients
MIT License
480 stars 147 forks source link

Incorrect parsing of `X-AUTHORIZATION-CONTENT-SHA256` header #197

Closed maxd closed 2 years ago

maxd commented 3 years ago

Look at this code:

uri = URI('http://localhost:3000')
request = Net::HTTP::Post.new(uri)
request.content_type = 'application/x-www-form-urlencoded'

signed_request = ApiAuth.sign!(request, 'test', MainController::SECRET_KEY)

response = Net::HTTP.start(uri.hostname, uri.port) do |http|
    http.request(signed_request)
end

It generate the following canonical_string for sign (see ApiAuth#hmac_signature):

POST,application/x-www-form-urlencoded,47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=,/,Tue, 27 Jul 2021 07:01:51 GMT

As you can see here is 47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU= part.

Now look at this code in Rails controller:

  before_action :api_authenticate

  def api_authenticate
    unless ApiAuth.authentic?(request, SECRET_KEY)
      head :unauthorized
    end
  end

It parse request and extract the following canonical_string for sign:

POST,application/x-www-form-urlencoded,,/,Tue, 27 Jul 2021 07:08:46 GMT

As you can see here isn't 47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU= part.

It's happen because api-auth incorrectly parse X-AUTHORIZATION-CONTENT-SHA256 header. Look at this method at ApiAuth::RequestDrivers::ActionControllerRequest:

def content_hash
    find_header(%w[X-AUTHORIZATION-CONTENT-SHA256])
end

As you can see it expect X-AUTHORIZATION-CONTENT-SHA256 header, BUT request contains custom headers in HTTP_* format and this method can't find this header by such name.

To fix this problem need to check several additional headers (by analogy with other methods i.e. original_uri):

def content_hash
    find_header(%w[X-AUTHORIZATION-CONTENT-SHA256 X_AUTHORIZATION_CONTENT_SHA256 HTTP_X_AUTHORIZATION_CONTENT_SHA256])
end

I'm going to provide PR to fix this problem.

taylorthurlow commented 3 years ago

@mgomes Do you mind reviewing this? I upgraded to 2.5.0 to fix deprecation warnings but this undocumented change to the header name lookup broke things.

mgomes commented 3 years ago

@maxd thanks for this PR. I think the best way forward from here would be to roll back the change in 2.5.0 and release 2.6.0. Then incorporate your PR into the 2.5.0 codebase but we release a version 3.0.0.

What do you think @maxd and @taylorthurlow?

taylorthurlow commented 3 years ago

Seems like the right way to do things 👍

maxd commented 3 years ago

@mgomes Do you want to rollback 72b64a0a0c8ba3461dde49b2fb20fdfae938802a commit (from v.2.5.0) because it broke compatibility with v.2.4.0 (don't support MD5 hashes) and then release v.2.6.0 with this rollback only. Am I right?

In this case, my PR will be useless because it fixes the problem introduced in this commit. Nevertheless, I think rollback this commit (and maybe refactor this feature in the future) is a good idea because it simplifies migrations to the next versions.