Implements request body validation inside HmacAuth #2419

vaibhavatul47 · 2017-04-19T20:21:09Z

Summary

Adding functionality to validate request body in hmac-auth. This is optional and can be configured when enabling the Hmac-Auth plugin for an API.

Full changelog

Currently, Hmac-Auth plugin allows validation of only request headers. There are requirements in real life projects when the validity of request body is also to be verified. To solve this problem I have introduced the usage of digest header. This digest header will contain the Base64 encoded Sha256 hash of the request body.

If an API has hmac-auth plugin enabled and validate-request-body config is enabled then for every request of this API received at KONG, Hmac-Auth plugin will also validate the integrity of request body, needless to say, this check will be performed only if request contains any body

If this config( validate-request-body) is enabled on an API then every request must contain digest header inside hmac-headers parameter.

Tests added

should pass with GET when body validation enabled
should pass with POST when body validation is enabled and digest header present
should not pass with POST when body validation enabled and digest header missing
~~should not pass with POST when body validation enabled and digest header missing from hmac-headers~~ ( This test will be added in Feature/check mandatory headers in hmac param #2418 )
should not pass with POST when body validation enabled and postBody is tampered
should not pass with POST when body validation enabled and digest header is tampered

vaibhavatul47 · 2017-04-19T20:23:58Z

This will fail in some tests as those depends on changes in PR: #2418
So those tests will pass once PR #2418 will be merged into this

p0pr0ck5 · 2017-04-20T15:29:46Z

@vaibhavatul47 please address the linting error and test failures: https://travis-ci.org/Mashape/kong/builds/223708709

p0pr0ck5 · 2017-04-26T20:25:36Z

@vaibhavatul47 there is still a failure in the test:

Plugin: hmac-auth (access) HMAC Authentication should not pass with POST when body validation enabled and digest header missing from hmac-headers

Mind taking a look?

vaibhavatul47 · 2017-05-02T04:34:29Z

@p0pr0ck5 This is expected behaviour as of now. The test which is failing depends on checking mandatory params for hmac-headers. This is implemented in PR #2418. I had already mentioned in above comments that few test cases will fail until #2418 will be merged.
Please also have a look at this comment on #2418

vaibhavatul47 · 2017-05-27T12:09:17Z

Hi @p0pr0ck5, I was busy in personal work so couldn't finish this sooner.
I have made required changes for this PR, i.e. I have removed failed tests from this PR. I would add them in #2418 once this PR is merged.

thibaultcha · 2017-05-30T19:42:25Z

@p0pr0ck5 and @shashiranjan84 will review this, as it is in need of a few improvements :) We will also need a documentation PR on getkong.org :) Thanks!

p0pr0ck5

Some small cleanup comments, nothing major.

p0pr0ck5 · 2017-05-31T16:44:03Z

kong/plugins/hmac-auth/access.lua

@@ -82,11 +87,26 @@ local function create_hash(request, hmac_params, headers)
  return ngx_sha1(hmac_params.secret, signing_string)
 end

+local function is_signature_equal(signature_1, signature_2)


what is the purpose of this function? constant time comparison? if so, please see 7345134 as to why this approach is incorrect; otherwise, please elaborate :)

Sorry, I mentioned the wrong commit. I should have typed 6ccdf7b :)

I've removed this function as no more required.

p0pr0ck5 · 2017-05-31T16:44:36Z

kong/plugins/hmac-auth/access.lua

@@ -152,7 +188,7 @@ local function set_consumer(consumer, credential)
  else
    ngx_set_header(constants.HEADERS.ANONYMOUS, true)
  end
-  
+


this change seems unwarranted? please remove it.

p0pr0ck5 · 2017-05-31T16:44:40Z

kong/plugins/hmac-auth/access.lua

@@ -173,10 +209,21 @@ local function do_authentication(conf)
  if not hmac_params.username then
    hmac_params = retrieve_hmac_fields(ngx.req, headers, AUTHORIZATION, conf)
  end
+


p0pr0ck5 · 2017-05-31T16:45:04Z

kong/plugins/hmac-auth/access.lua

@@ -189,7 +236,7 @@ local function do_authentication(conf)

  -- Retrieve consumer
  local consumer, err = cache.get_or_set(cache.consumer_key(credential.consumer_id),
-                   nil, load_consumer_into_memory, credential.consumer_id)
+    nil, load_consumer_into_memory, credential.consumer_id)


this whitespace change is incorrect, please remove it.

p0pr0ck5 · 2017-05-31T16:45:42Z

kong/plugins/hmac-auth/access.lua

 function _M.execute(conf)

  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
-    -- we're already authenticated, and we're configured for using anonymous, 
+    -- we're already authenticated, and we're configured for using anonymous,


please avoid whitespace changes like this, as it dirties the commit history and makes future archaeological efforts more challenging.

p0pr0ck5 · 2017-05-31T16:48:50Z

kong/plugins/hmac-auth/access.lua

@@ -213,7 +259,7 @@ function _M.execute(conf)
    if conf.anonymous ~= "" and conf.anonymous ~= nil then
      -- get anonymous user
      local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
-                       nil, load_consumer_into_memory, conf.anonymous, true)
+        nil, load_consumer_into_memory, conf.anonymous, true)


idem, please remove this unnecessary whitespace change.

p0pr0ck5 · 2017-05-31T22:44:34Z

kong/plugins/hmac-auth/access.lua

@@ -129,6 +149,22 @@ local function validate_clock_skew(headers, date_header_name, allowed_clock_skew
  return true
 end

+local function validate_request_body(headers, body)


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?

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.

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.

@vaibhavatul47 please also rebase to next.

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

I've fixed this.

LGTM

p0pr0ck5 · 2017-06-05T17:54:46Z

LGTM, @vaibhavatul47 would you be able to adjust your commit message based on https://github.com/Mashape/kong/blob/master/CONTRIBUTING.md#commit-message-format? @shashiranjan84 any thoughts here?

p0pr0ck5 · 2017-06-05T17:55:49Z

kong/plugins/hmac-auth/access.lua

+  local sha_recieved = headers[DIGEST]
+
+  if body == nil then
+    return true


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

shashiranjan84 · 2017-06-07T17:12:12Z

@vaibhavatul47 @p0pr0ck5 overall LGTM, I'll format it to latest code style before merge.

Currently, Hmac-Auth plugin allows validation of only request headers. There are requirements in real life projects when the validity of requestbody is also to be verified. To solve this problem I have introduced the usage of digest header. This digest header will contain the Base64 encoded Sha256 hash of the request body. If an API has hmac-auth plugin enabled and validate-request-body config is enabled then for every request of this API received at KONG, Hmac-Auth plugin will also validate the integrity of request body, needless to say, this check will be performed only if request contains any body. If this config( validate-request-body) is enabled on an API then every request must contain digest header inside hmac-headers parameter. Changes committed: modified: kong/plugins/hmac-auth/access.lua Added function which will validate request body modified: kong/plugins/hmac-auth/schema.lua added new config parameter: `validate_request_body` modified: spec/03-plugins/20-hmac-auth/03-access_spec.lua added multiple tests for this feature.

vaibhavatul47 · 2017-06-07T18:27:41Z

@p0pr0ck5 I've updated commit message.

shashiranjan84 · 2017-06-07T18:45:24Z

kong/plugins/hmac-auth/access.lua

+  if body == nil then
+    return true
+  elseif sha_recieved == nil then
+    return false


@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

I ll update the code and tests

@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.

@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.

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

shashiranjan84 · 2017-06-07T21:23:30Z

kong/plugins/hmac-auth/access.lua

@@ -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


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

@shashiranjan84 I'll update code according to this.

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

shashiranjan84 · 2017-06-08T23:13:12Z

Closing this PR in favor of #2613.

p0pr0ck5 · 2017-06-09T19:40:34Z

#2613 was merged, thank you @vaibhavatul47 for your contribution!

vaibhavatul47 mentioned this pull request Apr 19, 2017

Feature/request body validation #2407

Closed

p0pr0ck5 added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Apr 20, 2017

vaibhavatul47 force-pushed the feature/request_body_validation_in_hmac_plugin branch from 68dd044 to 3a1bc87 Compare April 21, 2017 10:51

vaibhavatul47 mentioned this pull request May 2, 2017

Feature/check mandatory headers in hmac param #2418

Closed

p0pr0ck5 self-assigned this May 30, 2017

thibaultcha assigned shashiranjan84 May 30, 2017

p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 30, 2017

p0pr0ck5 previously requested changes May 31, 2017

View reviewed changes

p0pr0ck5 reviewed May 31, 2017

View reviewed changes

p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jun 1, 2017

vaibhavatul47 force-pushed the feature/request_body_validation_in_hmac_plugin branch 2 times, most recently from 9cdee84 to 941c012 Compare June 1, 2017 21:39

p0pr0ck5 reviewed Jun 5, 2017

View reviewed changes

vaibhavatul47 force-pushed the feature/request_body_validation_in_hmac_plugin branch from 941c012 to 704f47d Compare June 7, 2017 18:24

shashiranjan84 reviewed Jun 7, 2017

View reviewed changes

shashiranjan84 mentioned this pull request Jun 8, 2017

feat(hmac) Implement request body validation #2613

Merged

shashiranjan84 closed this Jun 8, 2017

Implements request body validation inside HmacAuth #2419

Implements request body validation inside HmacAuth #2419

Conversation

vaibhavatul47 commented Apr 19, 2017 • edited Loading

Summary

Full changelog

Tests added

vaibhavatul47 commented Apr 19, 2017

p0pr0ck5 commented Apr 20, 2017

p0pr0ck5 commented Apr 26, 2017

vaibhavatul47 commented May 2, 2017 • edited Loading

vaibhavatul47 commented May 27, 2017

thibaultcha commented May 30, 2017

p0pr0ck5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhavatul47 Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

p0pr0ck5 commented Jun 5, 2017

Choose a reason for hiding this comment

shashiranjan84 commented Jun 7, 2017

vaibhavatul47 commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 commented Jun 8, 2017

p0pr0ck5 commented Jun 9, 2017

vaibhavatul47 commented Apr 19, 2017 •

edited

Loading

vaibhavatul47 commented May 2, 2017 •

edited

Loading

vaibhavatul47 Jun 1, 2017 •

edited

Loading

shashiranjan84 Jun 7, 2017 •

edited

Loading

shashiranjan84 Jun 8, 2017 •

edited

Loading