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.

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

Hmac auth body validation #3347

Merged
merged 6 commits into from
May 19, 2018

Conversation

mvanholsteijn
Copy link
Contributor

Summary

Changes the hmac-auth request validation logic to pass if:

  • there is no Digest and no body
  • there is Digest for an empty body and no body.
  • there is a Digest for the body and a body.

This is to make sure that we can put 'digest' in the enforce_headers for all requests.

Full changelog

  • added tests for scenarios
  • added logic in validate_body()

Issues resolved

Fix #3345
Fix #3346

@shashiranjan84
Copy link
Contributor

@mvanholsteijn thanks for PR, would you please rebase before we start reviewing ?

@mvanholsteijn
Copy link
Contributor Author

@shashiranjan84 Do you have any idea on when this PR might be reviewed?

if not body then
return false
-- if no body, calculate sha-256 over 0 bytes
body = ''
end
Copy link
Contributor

Choose a reason for hiding this comment

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

you can drop this check and just do

sha256:update(body or '')

Copy link
Contributor

@shashiranjan84 shashiranjan84 left a comment

Choose a reason for hiding this comment

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

Overall looks good, just requested minor change.

Copy link
Contributor

@shashiranjan84 shashiranjan84 left a comment

Choose a reason for hiding this comment

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

LGTM

@shashiranjan84 shashiranjan84 added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review labels May 9, 2018
@shashiranjan84
Copy link
Contributor

shashiranjan84 commented May 9, 2018

@shashiranjan84 shashiranjan84 added pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... and removed pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) labels May 10, 2018
mvanholsteijn added a commit to binxio/getkong.org that referenced this pull request May 13, 2018
mvanholsteijn added a commit to binxio/getkong.org that referenced this pull request May 13, 2018
@mvanholsteijn
Copy link
Contributor Author

@shashiranjan84 I added the documentation to getkong.org...

@shashiranjan84 shashiranjan84 added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... labels May 17, 2018
@shashiranjan84
Copy link
Contributor

Thanks @mvanholsteijn .

@thibaultcha thibaultcha merged commit 96071b4 into Kong:master May 19, 2018
@thibaultcha
Copy link
Member

@mvanholsteijn Thank you for the patch! If you haven't done so yet, be sure to grab your Contributor T-shirt :)

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request May 19, 2018
Tieske pushed a commit to Kong/docs.konghq.com that referenced this pull request May 28, 2018
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request May 30, 2018
hbagdi pushed a commit to Kong/docs.konghq.com that referenced this pull request May 31, 2018
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jun 28, 2018
ukiahsmith pushed a commit to Kong/docs.konghq.com that referenced this pull request Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
3 participants