Skip to content
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.

Sign up for GitHub

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

Implements request body validation inside HmacAuth #2419

Closed
wants to merge 1 commit into from
Closed

Implements request body validation inside HmacAuth #2419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions kong/plugins/hmac-auth/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,28 @@ local cache = require "kong.tools.database_cache"
local responses = require "kong.tools.responses"
local constants = require "kong.constants"
local singletons = require "kong.singletons"
local resty_sha256 = require "resty.sha256"

local math_abs = math.abs
local ngx_time = ngx.time
local ngx_gmatch = ngx.re.gmatch
local ngx_decode_base64 = ngx.decode_base64
local ngx_encode_base64 = ngx.encode_base64
local ngx_parse_time = ngx.parse_http_time
local ngx_sha1 = ngx.hmac_sha1
local ngx_set_header = ngx.req.set_header
local ngx_get_headers = ngx.req.get_headers
local ngx_log = ngx.log
local req_read_body = ngx.req.read_body
local req_get_body_data = ngx.req.get_body_data

local split = utils.split

local AUTHORIZATION = "authorization"
local PROXY_AUTHORIZATION = "proxy-authorization"
local DATE = "date"
local X_DATE = "x-date"
local DIGEST = "digest"
local SIGNATURE_NOT_VALID = "HMAC signature cannot be verified"

local _M = {}
Expand Down Expand Up @@ -83,10 +88,10 @@ local function create_hash(request, hmac_params, headers)
end

local function validate_signature(request, hmac_params, headers)
local digest = create_hash(request, hmac_params, headers)
local sig = ngx_decode_base64(hmac_params.signature)
local signature_1 = create_hash(request, hmac_params, headers)
local signature_2 = ngx_decode_base64(hmac_params.signature)

return digest == sig
return signature_1 == signature_2
end

local function load_credential_into_memory(username)
Expand Down Expand Up @@ -129,6 +134,22 @@ local function validate_clock_skew(headers, date_header_name, allowed_clock_skew
return true
end

local function validate_request_body(headers, body)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ietf draft indicates that a digest header may look something like:

digest: SHA-256=X48E9qOokqqrvdts8nOJRJN3OWDUoyWxBf7kbu9DBPE=\n

It doesn't appear that the generate hash will match such a format, with the form of the hash prepended to the base64-encoded hash itself. Do we care about this case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote my logic assuming Kong will always check the SHA-256 hash of request body.
As per ieft draft, the user can use any hash algorithm to generate the hash. If we allow this, then for each request, Kong would need to parse Digest header and then figure out which hashing algorithm to use. Then there could be cases where a user uses an algorithm not supported by Kong libraries.
So for simplicity, I assumed only SHA-256.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do one thing to abide by ieft draft.
At Kong, we can generate hash string prepended by the hash algorithm as recommended by draft, i.e.:
digest: SHA-256=X48E9qOokqqrvdts8nOJRJN3OWDUoyWxBf7kbu9DBPE=\n
Then we would need to mention it clearly that user needs to send digest header in this format and only sha-256 algorithm is supported by Kong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavatul47 please also rebase to next.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavatul47 i think this is a good approach, as it could allow us to support additional algorithms in the future.

Copy link
Author

@vaibhavatul47 vaibhavatul47 Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this.

local sha_recieved = headers[DIGEST]

if body == nil then
return true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style, needs a blank line after return. not a blocker, we can adjust this as part of merge

elseif sha_recieved == nil then
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavatul47 here your logic forcing client to send digest header. I would prefer logic to be

if validate_request_body and headers[digest] then
  validate digest
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ll update the code and tests

Copy link
Contributor

@shashiranjan84 shashiranjan84 Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavatul47 here is what I am thinking to change it to

-- If request body validation is enabled, request may require `digest` validation 
 local digest_recieved = headers[DIGEST]
  if conf.validate_request_body and digest_recieved then

    if not validate_digest(digest_recieved) then
      return false, {status = 403, message = SIGNATURE_NOT_VALID }
    end
  end


-----------------------
local function validate_digest(digest_recieved)

  req_read_body()
  local body = req_get_body_data()
  -- request must have body as client sent
  -- a digest header
  if not body then
    return false
  end

  local sha256 = resty_sha256:new()
  sha256:update(body)
  local digest_created = "SHA-256=" .. ngx_encode_base64(sha256:final())

  return digest_created == digest_recieved
end

I will also work on adding support for other hashing algorithm, at least SHA1 and SHA-256 for both Digest and HTTP signature validation. May be a new PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashiranjan84 if validate_request_body is enabled then forcing client to send digest header is important. If this is not mandated then someone who can change request body would also delete digest header and then this corrupted request would be validated by plugin as it didn't contain any digest header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavatul47 Not really, If signature is created using digest, it would not pass validation as it is tampered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

end

local sha256 = resty_sha256:new()
sha256:update(body)
local sha_created = "SHA-256=" .. ngx_encode_base64(sha256:final())

return sha_created == sha_recieved
end

local function load_consumer_into_memory(consumer_id, anonymous)
local result, err = singletons.dao.consumers:find { id = consumer_id }
if not result then
Expand Down Expand Up @@ -177,6 +198,16 @@ local function do_authentication(conf)
return false, {status = 403, message = SIGNATURE_NOT_VALID}
end

-- If request body validation is enabled, then verify hash of request body.
if conf.validate_request_body then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we should validate headers first and then body. As if header is tampered there is no point validating body.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashiranjan84 I'll update code according to this.

Copy link
Contributor

@shashiranjan84 shashiranjan84 Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't, I already updated the code. Planning to merge it today

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

-- retrieve request body
req_read_body()
local body = req_get_body_data()
if not validate_request_body(headers, body) then
return false, {status = 403, message = "HMAC signature cannot be verified, a valid Sha-256 digest header is required for HMAC Authentication"}
end
end

-- validate signature
local credential = load_credential(hmac_params.username)
if not credential then
Expand Down
1 change: 1 addition & 0 deletions kong/plugins/hmac-auth/schema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ return {
hide_credentials = { type = "boolean", default = false },
clock_skew = { type = "number", default = 300, func = check_clock_skew_positive },
anonymous = {type = "string", default = "", func = check_user},
validate_request_body = { type = "boolean", default = false },
}
}
139 changes: 139 additions & 0 deletions spec/03-plugins/20-hmac-auth/03-access_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ local cjson = require "cjson"
local crypto = require "crypto"
local helpers = require "spec.helpers"
local utils = require "kong.tools.utils"
local resty_sha256 = require "resty.sha256"

local hmac_sha1_binary = function(secret, data)
return crypto.hmac.digest("sha1", data, secret, true)
Expand Down Expand Up @@ -67,6 +68,20 @@ describe("Plugin: hmac-auth (access)", function()
}
})

local api4 = assert(helpers.dao.apis:insert {
name = "api-4",
hosts = { "hmacauth4.com" },
upstream_url = "http://mockbin.com"
})
assert(helpers.dao.plugins:insert {
name = "hmac-auth",
api_id = api4.id,
config = {
clock_skew = 3000,
validate_request_body = true
}
})

assert(helpers.start_kong {
real_ip_header = "X-Forwarded-For",
real_ip_recursive = "on",
Expand Down Expand Up @@ -847,6 +862,130 @@ describe("Plugin: hmac-auth (access)", function()
})
assert.response(res).has.status(500)
end)

it("should pass with GET when body validation enabled", function()
local date = os.date("!%a, %d %b %Y %H:%M:%S GMT")
local encodedSignature = ngx.encode_base64(hmac_sha1_binary("secret", "date: "..date))
local hmacAuth = [["hmac username="bob",algorithm="hmac-sha1",]]
..[[headers="date",signature="]]..encodedSignature..[["]]
local res = assert(client:send {
method = "GET",
path = "/requests",
body = {},
headers = {
["HOST"] = "hmacauth4.com",
date = date,
authorization = hmacAuth
}
})
assert.res_status(200, res)
end)

it("should pass with POST when body validation enabled and digest header present", function()
local date = os.date("!%a, %d %b %Y %H:%M:%S GMT")
local postBody = '{"a":"apple","b":"ball"}'
local sha256 = resty_sha256:new()
sha256:update(postBody)
local digest = "SHA-256=" .. ngx.encode_base64(sha256:final())

local encodedSignature = ngx.encode_base64(
hmac_sha1_binary("secret", "date: "..date.."\n".."digest: "..digest))
local hmacAuth = [["hmac username="bob",algorithm="hmac-sha1",]]
..[[headers="date digest",signature="]]..encodedSignature..[["]]
local res = assert(client:send {
method = "POST",
path = "/requests",
body = postBody,
headers = {
["HOST"] = "hmacauth4.com",
date = date,
digest = digest,
authorization = hmacAuth
}
})
assert.res_status(200, res)
end)

it("should not pass with POST when body validation enabled and digest header missing", function()
local date = os.date("!%a, %d %b %Y %H:%M:%S GMT")
local postBody = '{"a":"apple","b":"ball"}'
local sha256 = resty_sha256:new()
sha256:update(postBody)
local digest = "SHA-256=" .. ngx.encode_base64(sha256:final())

local encodedSignature = ngx.encode_base64(
hmac_sha1_binary("secret", "date: "..date.."\n".."digest: "..digest))
local hmacAuth = [["hmac username="bob",algorithm="hmac-sha1",]]
..[[headers="date digest",signature="]]..encodedSignature..[["]]
local res = assert(client:send {
method = "POST",
path = "/requests",
body = postBody,
headers = {
["HOST"] = "hmacauth4.com",
date = date,
authorization = hmacAuth
}
})
local body = assert.res_status(403, res)
body = cjson.decode(body)
assert.equal("HMAC signature cannot be verified, a valid Sha-256 digest header is required for HMAC Authentication", body.message)
end)

it("should not pass with POST when body validation enabled and postBody is tampered", function()
local date = os.date("!%a, %d %b %Y %H:%M:%S GMT")
local postBody = '{"a":"apple","b":"ball"}'
local sha256 = resty_sha256:new()
sha256:update(postBody)
local digest = "SHA-256=" .. ngx.encode_base64(sha256:final())

local encodedSignature = ngx.encode_base64(
hmac_sha1_binary("secret", "date: "..date.."\n".."digest: "..digest))
local hmacAuth = [["hmac username="bob",algorithm="hmac-sha1",]]
..[[headers="date digest",signature="]]..encodedSignature..[["]]
local res = assert(client:send {
method = "POST",
path = "/requests",
body = "abc",
headers = {
["HOST"] = "hmacauth4.com",
date = date,
digest = digest,
authorization = hmacAuth
}
})
local body = assert.res_status(403, res)
body = cjson.decode(body)
assert.equal("HMAC signature cannot be verified, a valid Sha-256 digest header is required for HMAC Authentication", body.message)
end)

it("should not pass with POST when body validation enabled and digest header is tampered", function()
local date = os.date("!%a, %d %b %Y %H:%M:%S GMT")
local postBody = '{"a":"apple","b":"ball"}'
local sha256 = resty_sha256:new()
sha256:update(postBody)
local digest = "SHA-256=" .. ngx.encode_base64(sha256:final())

local encodedSignature = ngx.encode_base64(
hmac_sha1_binary("secret", "date: "..date.."\n".."digest: "..digest))
local hmacAuth = [["hmac username="bob",algorithm="hmac-sha1",]]
..[[headers="date digest",signature="]]..encodedSignature..[["]]
local res = assert(client:send {
method = "POST",
path = "/requests",
body = postBody,
headers = {
["HOST"] = "hmacauth4.com",
date = date,
digest = "abc",
authorization = hmacAuth
}
})
local body = assert.res_status(403, res)
body = cjson.decode(body)
assert.equal("HMAC signature cannot be verified, a valid Sha-256 digest header is required for HMAC Authentication", body.message)
end)

end)
end)

Expand Down