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

[Filebeat] Enable HMAC Signature Validation for http_endpoint input #24918

Merged
merged 9 commits into from
May 13, 2021

Conversation

SpencerLN
Copy link
Contributor

@SpencerLN SpencerLN commented Apr 2, 2021

What does this PR do?

Adds the option of HMAC signature validation for the http_endpoint input. This PR adds support for sha1 and sha256 based HMAC validation and allows flexible configuration of the header containing the signature, the key, and an optional prefix.

Why is it important?

Many webhook senders offer this as the only method of authenticating that the method was actually sent from them, for example, GitHub and Dropbox. This enables future modules to receive events from these types of senders and allows users the flexibility to configure their own inputs in the meantime.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

This PR can be tested locally by configuring a webhook sender to send events to your machine. I personally used ngrok to expose the appropriate port from Filebeat, but there are other options. One simple sender to use for testing is configuring a webhook on any GitHub repo.

https://docs.github.com/en/developers/webhooks-and-events/creating-webhooks#exposing-localhost-to-the-internet

Related issues

@SpencerLN SpencerLN added enhancement needs_backport PR is waiting to be backported to other branches. labels Apr 2, 2021
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 2, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 2, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #24918 updated

  • Start Time: 2021-05-12T23:07:23.693+0000

  • Duration: 107 min 27 sec

  • Commit: 892b00e

Test stats 🧪

Test Results
Failed 0
Passed 7031
Skipped 1193
Total 8224

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 7031
Skipped 1193
Total 8224

@mergify
Copy link
Contributor

mergify bot commented Apr 7, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add-hmac-validation upstream/add-hmac-validation
git merge upstream/master
git push upstream add-hmac-validation

@SpencerLN SpencerLN changed the title Enable HMAC Signature Validation for http_endpoint input [Filebeat] Enable HMAC Signature Validation for http_endpoint input Apr 7, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 7, 2021
@marc-gr marc-gr requested a review from P1llus April 15, 2021 11:28
@andrewkroh andrewkroh requested a review from marc-gr May 3, 2021 14:49
@P1llus
Copy link
Member

P1llus commented May 10, 2021

@SpencerLN Would you mind fixing the conflict with the changelog?

Copy link
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

It does LGTM, have you tested this for example with Github @SpencerLN ?

It looks better than my older implementation here: https://github.com/elastic/beats/pull/20744/files

@SpencerLN
Copy link
Contributor Author

It does LGTM, have you tested this for example with Github @SpencerLN ?

It looks better than my older implementation here: https://github.com/elastic/beats/pull/20744/files

@P1llus I wrote this with GitHub in mind and that was what I did my primary testing with.

The conflict for the changelog should be resolved now; is there anything else I should take care of? I wasn't sure if I needed to add different labels for Mergify to work.

@P1llus
Copy link
Member

P1llus commented May 10, 2021

It does LGTM, have you tested this for example with Github @SpencerLN ?
It looks better than my older implementation here: https://github.com/elastic/beats/pull/20744/files

@P1llus I wrote this with GitHub in mind and that was what I did my primary testing with.

The conflict for the changelog should be resolved now; is there anything else I should take care of? I wasn't sure if I needed to add different labels for Mergify to work.

Just need to confirm a few things with @andrewkroh in terms of how the response body is read, and if we can still get this to work if we wanted to read the body further in the future (like body validation logic, middleware etc), (should be okay because of the NopCloser).

After that we can add the labels for you.

@mergify
Copy link
Contributor

mergify bot commented May 12, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add-hmac-validation upstream/add-hmac-validation
git merge upstream/master
git push upstream add-hmac-validation

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Thank you for including tests with this change.

x-pack/filebeat/input/http_endpoint/validate.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/http_endpoint/validate.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/http_endpoint/validate.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/http_endpoint/validate.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/http_endpoint/validate.go Outdated Show resolved Hide resolved
@andrewkroh andrewkroh force-pushed the add-hmac-validation branch from 0fe36d0 to d19ebd8 Compare May 12, 2021 23:02
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I pushed an update to address the comments I made.

I also changed error messages that were capitalized as an unrelated cleanup.

@andrewkroh andrewkroh merged commit 68d8948 into elastic:master May 13, 2021
@andrewkroh andrewkroh added the backport-v7.14.0 Automated backport with mergify label May 13, 2021
mergify bot pushed a commit that referenced this pull request May 13, 2021
…24918)

* Enable HMAC Signature Validation for http_endpoint input

* Fix error message for invalid HmacType

* Update changelog

* Correct variable names to better follow conventions

* Don't capitalize error messages

Error strings should not be capitalized. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

* Avoid manual JSON encoding

Use Go's JSON encoder to ensure proper escaping.

* Refactor HMAC validation

Validate the HMAC header before progressing to the HMAC calculation.

Avoid copying body contents twice.

* Fix changelog merge

* Add punctuation to docs

Co-authored-by: Andrew Kroh <[email protected]>
(cherry picked from commit 68d8948)
andrewkroh pushed a commit that referenced this pull request May 18, 2021
…24918) (#25702)

* Enable HMAC Signature Validation for http_endpoint input

* Fix error message for invalid HmacType

* Update changelog

* Correct variable names to better follow conventions

* Don't capitalize error messages

Error strings should not be capitalized. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

* Avoid manual JSON encoding

Use Go's JSON encoder to ensure proper escaping.

* Refactor HMAC validation

Validate the HMAC header before progressing to the HMAC calculation.

Avoid copying body contents twice.

* Fix changelog merge

* Add punctuation to docs

Co-authored-by: Andrew Kroh <[email protected]>
(cherry picked from commit 68d8948)

Co-authored-by: Spencer Niemi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.14.0 Automated backport with mergify enhancement needs_backport PR is waiting to be backported to other branches.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support HMAC Validation for Filebeat http_endpoint input
4 participants