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

Update docs on Generic/HMAC receiver payload #256

Open
tbondarchuk opened this issue Oct 8, 2021 · 1 comment
Open

Update docs on Generic/HMAC receiver payload #256

tbondarchuk opened this issue Oct 8, 2021 · 1 comment

Comments

@tbondarchuk
Copy link

I've been playing with generic/hmac receivers and found out that payload actually doesn't matter at all for them.

While generic webhook documentation states quite clearly that "the controller will not perform token validation nor event filtering.", the hmac webhook example kinda makes you think you must pass some payload as <request-body>. But I've tested with signing empty string and it works, even with GET request. Checked the source code:

case v1beta1.GenericHMACReceiver:

(and wow, even with my limited knowledge of golang I was able to understand what's going on, very nice 👍)
so payload is validated only for specific webhooks like github/gitlab, and validation is more or less just checking if event name matches, correct?

Anyway, maybe it's worth updating docs on hmac webhook to add clear statement that payload content is not validated, only signature is verified? or perhaps even update example with empty payload and/or GET instead of POST? Unless I'm missing something and singing empty string is not a good idea and any random text must be used?

P.S. The more I use FluxCD v2, the more I love it, many thanks for such an awesome product!

@lloydchang
Copy link

@aliusmiles I agree with you that the documentation needs to be updated.

In particular, the sentence

Notification controller validates the authenticity of the payload using HMAC

is misleading.

This relates to fluxcd/website#612 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants