Skip to content

Add Webhook recursion detection (step 1: logging)

Luke Duncalfe requested to merge 329743-web_hooks-recursion-limit into master

What does this MR do and why?

A user can create a webhook that is configured so that the trigger and webhook URL create an infinite response loop.

An example of this is https://gitlab.com/gitlab-org/security/gitlab/issues/61. https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/216 applied a method of blocking webhooks from firing for this particular scenario. However, we are still exposed to recursive webhooks being created, for example, for issue events, and others.

We would like to have blanket protection against all forms of (accidental) webhook loops but continue to support workflows that use webhooks to call the API non-recursively.

Logging

This MR prepares for blocking recursive webbooks by first logging when we detect them. We'll inspect the logs on production and check the assumptions around the limits before enabling the blocking.

Detection

The detection tracks webhooks triggered by other webhooks through headers.

Webhooks will make requests with a new header containing a UUID. If this special UUID header is present in the request to the instance, the value will be passed on to all webhooks that fire within that request to include in their own headers, and so on.

When a webhook is triggered it will be registered against this UUID, and due to the recycling of this value through headers, any webhooks that are triggered by the webhook will also be registered against the same UUID. Therefore, we can detect and log (and eventually block too) when a webhook is recursively triggering.

If the special UUID header was not present, webhooks will be registered against a new UUID and also pass it in their request header.

We also detect when the total number of webhooks registered against a UUID exceeds a limit, as a sanity limit.

#329743 (closed)

How to set up and validate locally

Setup

  1. To allow testing locally, allow webhooks requests to your localhost:
    1. Go Admin > Settings > Network.
    2. Expand Outbound requests.
    3. Check Allow requests to the local network from web hooks and services.
    4. Click Save changes.
  2. Enable the feature flag:
    Feature.enable(:webhook_recursion_detection)
  3. Create a private token.
  4. Choose a project that:
    1. you have admin rights to
    2. has a merge request
  5. Add a new Webhook:
    1. Go Settings > Webhooks.
    2. For URL add http://127.0.0.1:3000/api/v4/projects/<project-id>/merge_requests/<merge-request-iid>/add_spent_time?duration=3m&private_token=<your-token>, replacing the three placeholders with the correct details.
    3. For Trigger select "Merge request events".
    4. Uncheck Enable SSL verification.
    5. Save the Webhook.

Trigger the recursion

Go to the merge request and update its description. The Webhook should begin to fire recursively.

View the Webhook details

You can check that the recursion is happening by viewing the Webhook logs:

  1. Go Settings > Webhooks.
  2. Scroll down to your webhook, and click Edit.
  3. Scroll down to Recent events.

You can check that the headers are being passed from request to request and that the chain count header is being incremented.

  1. Click View details next to one of the logged events.
  2. Scroll down Headers, note the X-Gitlab-Event-UUID will be the same as the even before it.

View the logs

You can check that we log the detection:

  1. Tail the auth logs from your terminal: tail -f log/auth.log

How to disable the Webhook

Your webhook will continue to fire until you disable it. The easiest way is to edit it and change the private_token to be invalid, which will cause it to stop recursively firing.

Or you can apply this patch, which is similar to what the next step will be when we come to block them:

diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index 7b35db26fd5..59ef4327f0e 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -97,8 +97,7 @@ def execute
   def async_execute
     Gitlab::ApplicationContext.with_context(hook.application_context) do
       break log_rate_limit if rate_limited?
-
-      log_recursion_limit if recursion_blocked?
+      break log_recursion_limit if recursion_blocked?

       data[:_gitlab_recursion_detection] = Gitlab::WebHooks::RecursionDetection.to_h

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #329743 (closed)

Edited by Luke Duncalfe

Merge request reports