fix(hmac-auth) add deprecation transition for pr #3339 #3699
Conversation
Fix PR Kong#3339 by adding support for older signatures generated from `request-line` without the query_string. To reduce performance impact, calculate the new signature version first. Only if that first validation fails, try again with the deprecated signing function.
Only calculate deprecated hash if the new hash validation fails.
|
@mlehner616 Thanks for taking the initiative here! @shashiranjan84 Will you help review the proposed patch for this behaviour? Thanks! |
There was a problem hiding this comment.
@mlehner616 thanks for PR. Did a high level review, please address the comments I left and request you to add some tests.
kong/plugins/hmac-auth/access.lua
Outdated
| local function validate_signature(request, hmac_params, headers) | ||
| local signature_1 = create_hash(request, hmac_params, headers) | ||
| local signature_2 = ngx_decode_base64(hmac_params.signature) | ||
| local signature_1 = create_hash(request, hmac_params, headers) |
There was a problem hiding this comment.
we don't need extra space before assignment operator.
kong/plugins/hmac-auth/access.lua
Outdated
| local signature_1 = create_hash(request, hmac_params, headers) | ||
| local signature_2 = ngx_decode_base64(hmac_params.signature) | ||
| local signature_1 = create_hash(request, hmac_params, headers) | ||
| local signature_2 = ngx_decode_base64(hmac_params.signature) |
There was a problem hiding this comment.
Also we don't need to create both the signature at once, one should be fall back and only be created if needed.
There was a problem hiding this comment.
@shashiranjan84 Whitespace removed, My commit from yesterday should have included the deprecated signature creation into the conditional. This looks like it would do what you're describing.
There was a problem hiding this comment.
You are right. Thanks again for your effort, we just need to add test, which would be very similar to https://github.com/Kong/kong/blob/master/spec/03-plugins/20-hmac-auth/03-access_spec.lua#L1150, you would just need to create request signature without query-param
shashiranjan84
added
pr/changes requested
|
@shashiranjan84 I made the changes you requested, I haven't had a chance to write tests for this yet. |
Add test to validate request-line containing query params but with the signature generated without them.
shashiranjan84
added
pr/please review
and removed
pr/changes requested
|
@mlehner616 thanks, we will review it. |
There was a problem hiding this comment.
Thank you for the patch @mlehner616. We would love to ship it in our next release (0.14.1) next week. Would you mind addressing the feedback I provided? Thanks!
kong/plugins/hmac-auth/access.lua
Outdated
| if not header_value then | ||
| if header == "request-line" then | ||
| -- request-line in hmac headers list | ||
| local request_line = fmt("%s %s HTTP/%s", ngx.req.get_method(), ngx.var.uri, ngx.req.http_version()) |
There was a problem hiding this comment.
Instead of duplicating the whole function, we should be able to simply specify ngx.var.uri or ngx.var.request_uri as an argument. That would be preferable.
kong/plugins/hmac-auth/access.lua
Outdated
| return signature_1 == signature_2 | ||
| if signature_1 == signature_2 then | ||
| return true | ||
| else |
There was a problem hiding this comment.
There is no need for this else branch if the above branch already returns.
| @@ -1080,6 +1080,47 @@ for _, strategy in helpers.each_strategy() do | |||
| assert.res_status(200, res) | |||
| end) | |||
|
|
|||
| it("should pass with GET with request-line having query param but signed without query param", function() | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
thibaultcha
added
pr/changes requested
Refactor create_hash and validate_signature function to removed duplicate code. We now pass in the ngx.var.uri or the ngx.var.request_uri as an argument to create_hash.
Adding comment to regression test
|
@thibaultcha @shashiranjan84 Thanks for the feedback. I've committed changes that address your latest suggestions. Let me know if there are any other tweaks needed. Is there any chance this PR will get backported to a 0.13.2+ release? We're actually blocked on the 0.14 versions right now since we're not ready to drop API support. |
|
@mlehner616 We do not have plans for a 0.13.2 version so far.
Not sure what you mean by that. In 0.14, the API entity is still supported ( |
|
@thibaultcha Sorry, I thought I remember seeing 0.14 dropped API entities so it looks like I misread that section. Thanks for the clarification, I'll wait for the release to get cut and give it a shot. |
|
@mlehner616 Great, thank you! |
Summary
Add deprecation transition for PR #3339 by adding support for older signatures generated from
request-linewithout the query_string. This is intended to be a patch on 0.13.1 but may apply to 0.14+ although I haven't tested this.Full changelog
Issues resolved
Fix #3672