feat(hmac) multiple HMAC algorithms support and `enforce_headers` option #2644

shashiranjan84 · 2017-06-21T21:58:12Z

SUMMARY

Support for HMAC-SHA256, HMAC-SHA384, HMAC-SHA512.
User can enforce which headers must at least be used for
http signature creation using config.enforce_headers.

Full changelog

config.alogorithms and config.enforce_headers added to schema
Logic updated to support multiple algorithms and enforce headers
Tests cases added to test new logic

p0pr0ck5 · 2017-06-21T23:17:03Z

kong/plugins/hmac-auth/access.lua

+  local matched = false
+  for _, algo in ipairs(conf.algorithms) do
+    if algo == params.algorithm then
+      matched = true


lets just return true, no need to keep running through this loop once we have a match? also means we dont need matched:

for _, algo in ipairs(conf.algorithms) do if algo == params.algorithm then return true end end return nil, fmt("algorithm %s not supported", params.algorithm)

p0pr0ck5 · 2017-06-21T23:41:34Z

kong/plugins/hmac-auth/access.lua

+  end
+
+  -- check enforced headers are present
+  local enfoprced_header_set = list_as_set(conf.enforce_headers)


typo, 'enfoprced_header_set'.

also, how hard would it be to cache the enforce_headers table, so we dont have to constantly call list_as_set?

p0pr0ck5 · 2017-06-21T23:41:41Z

kong/plugins/hmac-auth/access.lua

+local function list_as_set(list)
+  local set = {}
+  for _, v in ipairs(list) do
+    set[v] = true;


Java memory :)

p0pr0ck5 · 2017-06-21T23:48:23Z

kong/plugins/hmac-auth/migrations/postgres.lua

+      for _, row in ipairs(rows) do
+        row.config.validate_request_body = false
+        row.config.enforce_headers = {}
+        row.config.algorithms = {"hmac-sha1"}


not quite sure i agree with this default assignment, as the schema default is to support all 4 algorithms.

This is intentional, older version only has support for hmac-sha1, so after migration I defaulted it to {"hmac-sha1"}. If user want to support all 4, he would need to patch the plugin.

p0pr0ck5 · 2017-06-21T23:50:07Z

kong/plugins/hmac-auth/access.lua

+}
+
+local function list_as_set(list)
+  local set = {}


since we know the size of this table, a minor optimization here would be to use table.new to create the appropriately-sized table.

p0pr0ck5

thanks! couple minor things then i think looks good!

p0pr0ck5 · 2017-06-22T15:43:08Z

kong/plugins/hmac-auth/access.lua

 local resty_sha256 = require "resty.sha256"
+local new_table = require "table.new"


we typically wrap this in pcall. see https://github.com/Mashape/kong/blob/master/kong/plugins/ip-restriction/handler.lua#L5 as an example

p0pr0ck5 · 2017-06-22T15:43:55Z

kong/plugins/hmac-auth/access.lua

+  end
+
+  -- check supported alorithm used
+  local matched = false


dont need this matched var. and after this for loop we can just return nil, fmt(...)

shashiranjan84 · 2017-06-22T16:19:10Z

@p0pr0ck5 makes sense, thanks. Updated code as you advised.

p0pr0ck5

A few minor things to cleanup, but LGTM overall.

p0pr0ck5 · 2017-06-22T22:30:58Z

kong/plugins/hmac-auth/migrations/cassandra.lua

+      for _, row in ipairs(rows) do
+        row.config.validate_request_body = false
+        row.config.enforce_headers = {}
+        row.config.algorithms = {"hmac-sha1"}


space in between brackets and quotes

p0pr0ck5 · 2017-06-22T22:32:21Z

spec/03-plugins/20-hmac-auth/01-schema_spec.lua

+  it("errors with wrong algorithm", function()
+    local ok, err = validate_entity({algorithms = {"sha1024"}}, hmac_auth_schema)
+    assert.equal('"sha1024" is not allowed. Allowed values are: "hmac-sha1", "hmac-sha256", "hmac-sha384", "hmac-sha512"', err.algorithms)
+    assert.False(ok)


prefer lowercase is_false to False, i believe

thibaultcha · 2017-06-23T01:17:27Z

kong/plugins/hmac-auth/access.lua

+}
+
+local function list_as_set(list)
+  local set = new_tab(#list, 0)


new_tab(narr, nrec), this should be: new_tab(0, #list)

Are you sure @thibaultcha, because in following code, 1st argument is length?

https://github.com/Mashape/kong/blob/master/kong/plugins/ip-restriction/handler.lua#L26

Yes, because this code you pointed is using the array part of the table, but your use-case concerns the hash part.

thibaultcha · 2017-06-23T01:18:33Z

kong/plugins/hmac-auth/access.lua

+
+local function validate_params(params, conf)
+  -- check username and signature are present
+  if not (params.username and params.signature) then


No parenthesis is the preferred style and also reads better: if not params.username or not params.signature

thibaultcha · 2017-06-23T01:19:10Z

kong/plugins/hmac-auth/access.lua

+    for _, header in ipairs(params.hmac_headers) do
+      enforced_header_set[header] = nil
+    end
+


one too many blank line it seems?

thibaultcha · 2017-06-23T01:19:45Z

kong/plugins/hmac-auth/access.lua

+      return true
+    end
+  end
+  return nil, fmt("algorithm %s not supported", params.algorithm)


We should jump a line before the return statement

thibaultcha · 2017-06-23T01:20:40Z

kong/plugins/hmac-auth/access.lua

 local _M = {}

+local hmac = {
+  ["hmac-sha1"] = function(secret, data)
+    return crypto.hmac.digest("sha1", data, secret, true)


Maybe we should keep using ngx.sha1 when possible? This way we reduce the risk and we take advantage of using the FFI over the crypto lib?

thibaultcha · 2017-06-23T01:20:52Z

kong/plugins/hmac-auth/access.lua

@@ -183,6 +248,7 @@ local function set_consumer(consumer, credential)
 end

 local function do_authentication(conf)
+


Seems this is an undesired change too?

thibaultcha · 2017-06-23T01:21:42Z

kong/plugins/hmac-auth/access.lua

    return false, {status = 403, message = SIGNATURE_NOT_VALID}
  end

  -- validate signature
  local credential = load_credential(hmac_params.username)
  if not credential then
+    ngx_log(ngx.DEBUG, "failed to retrieve credential for " .. hmac_params.username)


We should avoid string concatenation operators and use variadic arguments when possible. Here:

ngx_log(ngx.DEBUG, "failed to retrieve credential for ", hmac_params.username)

- Support for HMAC-SHA256, HMAC-SHA384, HMAC-SHA512. - User can enforce which headers must at least be used for http signature creation using `config.enforce_headers`.

shashiranjan84 · 2017-06-23T02:32:48Z

@thibaultcha update PR as you suggested.

updated

shashiranjan84 added the pr/status/please review label Jun 21, 2017

shashiranjan84 added this to the 0.11.0 milestone Jun 21, 2017

shashiranjan84 requested a review from p0pr0ck5 June 21, 2017 21:58

shashiranjan84 force-pushed the feat/hmac-header-algo branch from 92492ab to 8d55afa Compare June 21, 2017 22:21

p0pr0ck5 suggested changes Jun 21, 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 21, 2017

shashiranjan84 force-pushed the feat/hmac-header-algo branch from 8d55afa to cbf0cfe Compare June 22, 2017 02:06

shashiranjan84 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 Jun 22, 2017

p0pr0ck5 suggested changes Jun 22, 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 22, 2017

shashiranjan84 force-pushed the feat/hmac-header-algo branch from cbf0cfe to e52b7d5 Compare June 22, 2017 16:17

shashiranjan84 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 Jun 22, 2017

p0pr0ck5 previously approved these changes Jun 22, 2017

View reviewed changes

p0pr0ck5 removed the pr/status/please review label Jun 22, 2017

shashiranjan84 dismissed p0pr0ck5’s stale review via 046bd3e June 22, 2017 23:02

shashiranjan84 force-pushed the feat/hmac-header-algo branch from e52b7d5 to 046bd3e Compare June 22, 2017 23:02

p0pr0ck5 previously approved these changes Jun 22, 2017

View reviewed changes

p0pr0ck5 added the pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) label Jun 22, 2017

thibaultcha modified the milestones: 0.11.0, 0.11.0rc1 Jun 23, 2017

thibaultcha previously requested changes Jun 23, 2017

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) labels Jun 23, 2017

thibaultcha modified the milestones: 0.11.0rc1, 0.11.0 Jun 23, 2017

feat(hmac) multiple HMAC algorithms support and enforce_headers option

4b18109

- Support for HMAC-SHA256, HMAC-SHA384, HMAC-SHA512. - User can enforce which headers must at least be used for http signature creation using `config.enforce_headers`.

shashiranjan84 dismissed p0pr0ck5’s stale review via 4b18109 June 23, 2017 02:30

shashiranjan84 force-pushed the feat/hmac-header-algo branch from 046bd3e to 4b18109 Compare June 23, 2017 02:30

shashiranjan84 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 Jun 23, 2017

remove blankline

0f27df4

Tieske approved these changes Jun 23, 2017

View reviewed changes

Tieske merged commit 0da9be8 into next Jun 23, 2017

Tieske deleted the feat/hmac-header-algo branch June 23, 2017 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hmac) multiple HMAC algorithms support and `enforce_headers` option #2644

feat(hmac) multiple HMAC algorithms support and `enforce_headers` option #2644

shashiranjan84 commented Jun 21, 2017 •

edited

Loading

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

shashiranjan84 Jun 22, 2017

p0pr0ck5 Jun 21, 2017

shashiranjan84 Jun 22, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 left a comment

p0pr0ck5 Jun 22, 2017

p0pr0ck5 Jun 22, 2017

shashiranjan84 commented Jun 22, 2017 •

edited

Loading

p0pr0ck5 left a comment

p0pr0ck5 Jun 22, 2017

p0pr0ck5 Jun 22, 2017

thibaultcha Jun 23, 2017

shashiranjan84 Jun 23, 2017

thibaultcha Jun 23, 2017

thibaultcha Jun 23, 2017

thibaultcha Jun 23, 2017

thibaultcha Jun 23, 2017

thibaultcha Jun 23, 2017

thibaultcha Jun 23, 2017

thibaultcha Jun 23, 2017

shashiranjan84 commented Jun 23, 2017

		local resty_sha256 = require "resty.sha256"
		local new_table = require "table.new"

		@@ -183,6 +248,7 @@ local function set_consumer(consumer, credential)
		end

		local function do_authentication(conf)

feat(hmac) multiple HMAC algorithms support and enforce_headers option #2644

feat(hmac) multiple HMAC algorithms support and enforce_headers option #2644

Conversation

shashiranjan84 commented Jun 21, 2017 • edited Loading

SUMMARY

Full changelog

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

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

shashiranjan84 commented Jun 22, 2017 • edited Loading

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

shashiranjan84 commented Jun 23, 2017

feat(hmac) multiple HMAC algorithms support and `enforce_headers` option #2644

feat(hmac) multiple HMAC algorithms support and `enforce_headers` option #2644

shashiranjan84 commented Jun 21, 2017 •

edited

Loading

shashiranjan84 commented Jun 22, 2017 •

edited

Loading