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

Jump to bottom

Feature/check mandatory headers in hmac param #2418

Closed
wants to merge 2 commits into from
Closed

Feature/check mandatory headers in hmac param #2418

wants to merge 2 commits into from

Conversation

vaibhavatul47
Copy link

Summary

Making date or x-date header mandatory in Hmac-headers.

Full changelog

Mandatory header values in Hmac-headers parameter

Currently, although the plugin validates clock-skew but it is no guarantee that the date or x-date header used in clock skew validation is not tampered.
Also, according to ietf draft it is madatory to include date headers in Hmac-header parameter.
If validate-request-body config is enabled for an API then Hmac-header should also must contain digest header.
Note: validate-request-body config is not present in this code. I'll add it in another PR as it is associated with another feature.

Test edited

  • should not pass with GET with date header missing from hmac-headers

Currently, although the plugin validates clock-skew but it is no guar…
6b319b7 
…antee

that the date or x-date header used in clock skew validation is not
tampered. Also, according to ietf draft
(https://tools.ietf.org/html/draft-cavage-http-signatures-06#section-2.3)
it is madatory to include date headers in Hmac-header parameter.

If validate-request-body config is enabled for an API then Hmac-header
should also must contain digest header.
Note: This config parameter is not present in this commit. I'll add it
in a different PR as it a totally different feature.

Changes committed:
    modified:   kong/plugins/hmac-auth/access.lua
        added a function which will check mandatory hmac params

    modified:   spec/03-plugins/20-hmac-auth/03-access_spec.lua
        1. added date header in Hmac-header parameter
        2. If date header passes skew test but this header was not used
           in Hmac-params then it is of no use. The header used in hmac-header
           should be used for skew test. Same for x-date header.
        3. added a test to fail request if date header not used in hmac-header
@vaibhavatul47 vaibhavatul47 changed the base branch from master to next April 19, 2017 20:16
This was referenced Apr 19, 2017
Closed
Closed
@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented May 1, 2017

Changes to the codebase should try to be as atomic as possible. Is it possible to re-work this PR such that it does not rely on a feature present in another branch?

@p0pr0ck5 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 May 1, 2017
@vaibhavatul47
Copy link
Author

@p0pr0ck5 I can remove validate-request-body part from this PR. Then, once this PR is merged, I'll work on PR #2419 and make corresponding changes in that PR.
This will suffice your atomic condition for PR. #2418 will comprise of changes related to mandating hmac-headers and #2419 will comprise of request body validation changes.

@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented May 5, 2017

@vaibhavatul47 ok, sounds good. will you make these changes?

@vaibhavatul47
Copy link
Author

@p0pr0ck5 sure, I'll update once its ready.

@thibaultcha thibaultcha 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
@shashiranjan84
Copy link
Contributor

@vaibhavatul47 here client has option to include Date or X-Date header as part of signing string. Here onus is on client to include Date or X-Date not Kong to mandate it.

thibaultcha
thibaultcha reviewed May 31, 2017
View reviewed changes
kong/plugins/hmac-auth/access.lua
@@ -13,21 +13,24 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

@p0pr0ck5 @shashiranjan84 Will you make sure that this is using the recent public API draft to read the request body as well? Thanks!

@vaibhavatul47
Copy link
Author

vaibhavatul47 commented Jun 2, 2017 •
edited
Loading

@shashiranjan84 Yes client would still have the option to chose from Date or X-Date header, but as per ietf draft, at least one of them must be used in hmac signing string generation

@shashiranjan84
Copy link
Contributor

shashiranjan84 commented Jun 2, 2017 •
edited
Loading

@vaibhavatul47 no as per draft, it is must for client to send the date header, not the server.

"In order to generate the string that is signed with a key, the client
MUST use the values of each HTTP header field in the headers
Signature parameter, in the order they appear in the headers
Signature parameter. It is out of scope for this document to dictate
what header fields an application will want to enforce, but
implementers SHOULD at minimum include the request target and Date
header fields."

@vaibhavatul47
Copy link
Author

@shashiranjan84 I never meant that server should send date header. I'm trying to say, whatever request client is sending, it must contain a header with date information.

@shashiranjan84
Copy link
Contributor

shashiranjan84 commented Jun 2, 2017 •
edited
Loading

@vaibhavatul47 Sorry I meant, It's on client to make sure to add date and request-line in order to make http-signature, not on Kong to force client to include these headers.

@shashiranjan84
Copy link
Contributor

I would prefer updating doc, to inform user to make sure to include date and request-line in signing string, over forcing it from Kong side.

@vaibhavatul47
Copy link
Author

@shashiranjan84 I think you are right, we should not mandate date header for following two reasons:

  1. The change is not backwards compatible. If a user is currently using Kong without date header in hmac signature, then she/he would face validation error.
  2. Not all users are concerned about replay attacks so it makes sense to leave it to users to include date header in hmac signature as per their requirement.

I will update my code to following:

  1. date or x-date headers won't be mandatory.
  2. If validate-request-body config is enabled, then only digest header will be mandated.

Also, we will need to update in docs to encourage users to include date header in the signature.

@shashiranjan84
Copy link
Contributor

@vaibhavatul47 I am thinking to close this PR and update your another PR #2419 to make it work independent of this PR. I will take care of the addition changes.

@shashiranjan84
Copy link
Contributor

As per discussion before, I am closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha left review comments

Assignees

@p0pr0ck5 p0pr0ck5

@shashiranjan84 shashiranjan84

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vaibhavatul47 @p0pr0ck5 @shashiranjan84 @thibaultcha