Add rate limiter for receiver

[phillebaba]

Exposing the receiver to the internet may result in an accidental dos of the api server, either through a bug in the other end making a request to the receiver or a bad actor figuring out the receiver endpoint.

The rate limiter is implemented in the events server in #167 but was not implemented for the receiver. The reason for this was because there is a high risk that too many requests would be rate limited as it is hard to differentiate requests for different commit references.

[phillebaba]

So Github includes the sha of the commit as a payload in the webook for push events.
https://docs.github.com/en/developers/webhooks-and-events/webhook-events-and-payloads#push

Hopefully other git providers do the same and we could be able to parse the different git provider payloads to rate limit. It would still break the receiver for other sources calling it however. What do you think about doing it this way @stefanprodan? We would have to find some other standard payload which other applications triggering receiver should send.

[lloydchang]

@phillebaba @stefanprodan Is this rate limiter required?

Risk mitigation for DOS can be addressed by the ingress' rate limiting, such as https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#rate-limiting

My understanding is the applications listed at https://toolkit.fluxcd.io/components/notification/controller/ have different webhook formats. You wouldn't find a standard payload amongst them.

Thank you @phillebaba @stefanprodan

[bigkevmcd]

@phillebaba I'd agree with @lloydchang relying on the payload signing to filter out DOS/rate-limiting is not a great idea, you need to load (and possibly cache) the shared-secret, then to read the entire body, and validate the signature.

I think it's better done at the ingress, but for accidental cases, require authentication, and then rate-limit the token is probably better.

[stefanprodan]

I'm ok with dropping this, indeed people should use an ingress and not expose the receiver with an L4 LB. We should update the docs here and tell users to use ingress for production setups.

[phillebaba]

I am fine with closing this issue, I created it to track the remaining work after we decided to drop it from the initial rate limiting feature. @lloydchang I do agree that having to support all possible combinations webhook formats would be cumbersome, so it is better if the end user deals with this for their specific use case. Lets update the docs and close this when that is done.