Skip to content

Commit

Permalink
Merge pull request from GHSA-45hq-g76r-46wv
Browse files Browse the repository at this point in the history 
Problems:

- We were automatically creating an integration when a webhook was received, this integration didn't have a secret set.
- The identifier of integrations (or better said, their URL) is really easy to guess. And for those created manually, the URL is the only protection they have.

Solutions and improvements:

- No longer create integrations automatically.
- All integrations are created with a secret attached to it.
- Use django's get_random_string to generate the secret, and generate one on save if the integration doesn't have one.
- Show the integration's secret in the detail view.
- Bitbucket now allows attaching a secret to their webhooks! https://bitbucket.org/blog/enhanced-webhook-security
- Use the sha256 signature to validate payloads from github (the sha1 version is deprecated)
- This doesn't break the gitea workaround, since they also support using a shared secret and use the same header as github to attach the signature.
- Old integrations that don't have a secret attached to them are still valid,
  but their functionality has been reduced to just trigger builds on existing versions,
  in order to trigger builds from PRs, and update the default branch, the webhook needs to be re-created with a secret.

Ref GHSA-45hq-g76r-46wv
  • Loading branch information
@stsewd
stsewd authored Nov 14, 2023
1 parent 6538d98 commit 55511b4
Show file tree
Hide file tree
Showing 14 changed files with 423 additions and 276 deletions.
20 changes: 12 additions & 8 deletions docs/user/guides/setup/git-repo-manual.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ Read the Docs will sync versions and build your documentation when your Git repo
You may need to prepend *https://* to the URL.
* For **Content type**, both *application/json* and
*application/x-www-form-urlencoded* work
* Leave the **Secrets** field blank
* Fill the **Secret** field with the value from the integration on Read the Docs
* Select **Let me select individual events**,
and mark **Branch or tag creation**, **Branch or tag deletion**, **Pull requests** and **Pushes** events
* Ensure **Active** is enabled; it is by default
Expand All @@ -55,21 +55,21 @@ Read the Docs will sync versions and build your documentation when your Git repo
If you see a Response 200, then the webhook is correctly configured.
For a 403 error, it's likely that the Payload URL is incorrect.

.. note:: The webhook token, intended for the GitHub **Secret** field, is not yet implemented.

.. tab:: Bitbucket

* Go to the :guilabel:`Settings` > :guilabel:`Webhooks` > :guilabel:`Add webhook` page for your project
* For **URL**, use the URL of the integration on Read the Docs,
found on the :guilabel:`Admin` > :guilabel:`Integrations` page
* Under **Triggers**, **Repository push** should be selected
* Fill the **Secret** field with the value from the integration on Read the Docs
* Finish by clicking **Save**

.. tab:: GitLab

* Go to the :guilabel:`Settings` > :guilabel:`Webhooks` page for your GitLab project
* For **URL**, use the URL of the integration on **Read the Docs project**,
found on the :guilabel:`Admin` > :guilabel:`Integrations` page
* Fill the **Secret token** field with the value from the integration on Read the Docs
* Leave the default **Push events** selected,
additionally mark **Tag push events** and **Merge request events**.
* Finish by clicking **Add Webhook**
Expand Down Expand Up @@ -99,7 +99,7 @@ Read the Docs will sync versions and build your documentation when your Git repo
* Leave the default **HTTP Method** as POST
* For **Content type**, both *application/json* and
*application/x-www-form-urlencoded* work
* Leave the **Secret** field blank
* Fill the **Secret** field with the value from the integration on Read the Docs
* Select **Choose events**,
and mark **Branch or tag creation**, **Branch or tag deletion** and **Push** events
* Ensure **Active** is enabled; it is by default
Expand Down Expand Up @@ -205,10 +205,14 @@ will be performed to ensure the authenticated user is an owner of the project.
Payload validation
------------------

If your project was imported through a connected account,
we create a secret for every integration that offers a way to verify that a webhook request is legitimate.
Currently, `GitHub <https://developer.github.com/webhooks/securing/>`__ and `GitLab <https://docs.gitlab.com/ee/user/project/integrations/webhooks.html#validate-payloads-by-using-a-secret-token>`__
offer a way to check this.
All integrations are created with a secret token,
this offers a way to verify that a webhook request is legitimate.

This validation is done according to each provider:

- `GitHub <https://developer.github.com/webhooks/securing/>`__
- `GitLab <https://docs.gitlab.com/ee/user/project/integrations/webhooks.html#validate-payloads-by-using-a-secret-token>`__
- `Bitbucket <https://support.atlassian.com/bitbucket-cloud/docs/manage-webhooks/#Secure-webhooks>`__

Troubleshooting
---------------
Expand Down
169 changes: 93 additions & 76 deletions readthedocs/api/v2/views/integrations.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import structlog
from django.shortcuts import get_object_or_404
from django.utils.crypto import constant_time_compare
from rest_framework import permissions, status
from rest_framework.exceptions import NotFound, ParseError
from rest_framework.renderers import JSONRenderer
Expand All @@ -29,8 +30,8 @@

log = structlog.get_logger(__name__)

GITHUB_EVENT_HEADER = "HTTP_X_GITHUB_EVENT"
GITHUB_SIGNATURE_HEADER = "HTTP_X_HUB_SIGNATURE"
GITHUB_EVENT_HEADER = "GitHub-Event"
GITHUB_SIGNATURE_HEADER = "X-Hub-Signature-256"
GITHUB_PING = "ping"
GITHUB_PUSH = "push"
GITHUB_PULL_REQUEST = "pull_request"
Expand All @@ -46,11 +47,12 @@
GITLAB_MERGE_REQUEST_OPEN = "open"
GITLAB_MERGE_REQUEST_REOPEN = "reopen"
GITLAB_MERGE_REQUEST_UPDATE = "update"
GITLAB_TOKEN_HEADER = "HTTP_X_GITLAB_TOKEN"
GITLAB_TOKEN_HEADER = "X-GitLab-Token"
GITLAB_PUSH = "push"
GITLAB_NULL_HASH = "0" * 40
GITLAB_TAG_PUSH = "tag_push"
BITBUCKET_EVENT_HEADER = "HTTP_X_EVENT_KEY"
BITBUCKET_EVENT_HEADER = "X-Event-Key"
BITBUCKET_SIGNATURE_HEADER = "X-Hub-Signature"
BITBUCKET_PUSH = "repo:push"


Expand Down Expand Up @@ -142,35 +144,35 @@ def is_payload_valid(self):
"""Validates the webhook's payload using the integration's secret."""
return False

@staticmethod
def get_digest(secret, msg):
"""Get a HMAC digest of `msg` using `secret`."""
digest = hmac.new(
secret.encode(),
msg=msg.encode(),
digestmod=hashlib.sha256,
)
return digest.hexdigest()

def get_integration(self):
"""
Get or create an inbound webhook to track webhook requests.
We shouldn't need this, but to support legacy webhooks, we can't assume
that a webhook has ever been created on our side. Most providers don't
pass the webhook ID in either, so we default to just finding *any*
integration from the provider. This is not ideal, but the
:py:class:`WebhookView` view solves this by performing a lookup on the
integration instead of guessing.
Most providers don't pass the webhook ID in either, so we default
to just finding *any* integration from the provider. This is not ideal,
but the :py:class:`WebhookView` view solves this by performing a lookup
on the integration instead of guessing.
"""
# `integration` can be passed in as an argument to `as_view`, as it is
# in `WebhookView`
if self.integration is not None:
return self.integration
try:
integration = Integration.objects.get(
project=self.project,
integration_type=self.integration_type,
)
except Integration.DoesNotExist:
integration = Integration.objects.create(
project=self.project,
integration_type=self.integration_type,
# If we didn't create the integration,
# we didn't set a secret.
secret=None,
)
return integration
self.integration = get_object_or_404(
Integration,
project=self.project,
integration_type=self.integration_type,
)
return self.integration

def get_response_push(self, project, branches):
"""
Expand Down Expand Up @@ -293,12 +295,15 @@ def update_default_branch(self, default_branch):
In case the user already selected a `default-branch` from the "Advanced settings",
it does not override it.
This action can be performed only if the integration has a secret,
requests from anonymous users are ignored.
"""
if not self.project.default_branch:
(
self.project.versions.filter(slug=LATEST).update(
identifier=default_branch
)
if self.get_integration().secret and not self.project.default_branch:
# Always check for the machine attribute, since latest can be user created.
# RTD doesn't manage those.
self.project.versions.filter(slug=LATEST, machine=True).update(
identifier=default_branch
)


Expand Down Expand Up @@ -371,31 +376,21 @@ def is_payload_valid(self):
See https://developer.github.com/webhooks/securing/.
"""
signature = self.request.META.get(GITHUB_SIGNATURE_HEADER)
signature = self.request.headers.get(GITHUB_SIGNATURE_HEADER)
secret = self.get_integration().secret
if not secret:
log.debug('Skipping payload signature validation.')
return True
if not signature:
return False
msg = self.request.body.decode()
digest = GitHubWebhookView.get_digest(secret, msg)
digest = WebhookMixin.get_digest(secret, msg)
result = hmac.compare_digest(
b'sha1=' + digest.encode(),
b"sha256=" + digest.encode(),
signature.encode(),
)
return result

@staticmethod
def get_digest(secret, msg):
"""Get a HMAC digest of `msg` using `secret`."""
digest = hmac.new(
secret.encode(),
msg=msg.encode(),
digestmod=hashlib.sha1,
)
return digest.hexdigest()

def handle_webhook(self):
"""
Handle GitHub webhook events.
Expand Down Expand Up @@ -429,7 +424,7 @@ def handle_webhook(self):
action = self.data.get('action', None)
created = self.data.get('created', False)
deleted = self.data.get('deleted', False)
event = self.request.META.get(GITHUB_EVENT_HEADER, GITHUB_PUSH)
event = self.request.headers.get(GITHUB_EVENT_HEADER, GITHUB_PUSH)
log.bind(webhook_event=event)
webhook_github.send(
Project,
Expand All @@ -452,16 +447,20 @@ def handle_webhook(self):
log.debug('Triggered sync_versions.')
return self.sync_versions_response(self.project)

# Handle pull request events
if self.project.external_builds_enabled and event == GITHUB_PULL_REQUEST:
if (
action in
[
GITHUB_PULL_REQUEST_OPENED,
GITHUB_PULL_REQUEST_REOPENED,
GITHUB_PULL_REQUEST_SYNC
]
):
integration = self.get_integration()

# Handle pull request events.
# Requests from anonymous users are ignored.
if (
integration.secret
and self.project.external_builds_enabled
and event == GITHUB_PULL_REQUEST
):
if action in [
GITHUB_PULL_REQUEST_OPENED,
GITHUB_PULL_REQUEST_REOPENED,
GITHUB_PULL_REQUEST_SYNC,
]:
# Trigger a build when PR is opened/reopened/sync
return self.get_external_version_response(self.project)

Expand All @@ -474,7 +473,6 @@ def handle_webhook(self):
event == GITHUB_PUSH,
(created or deleted),
]):
integration = self.get_integration()
events = integration.provider_data.get('events', []) if integration.provider_data else [] # noqa
if any([
GITHUB_CREATE in events,
Expand Down Expand Up @@ -556,14 +554,12 @@ def is_payload_valid(self):
It is sent in the request's header.
See https://docs.gitlab.com/ee/user/project/integrations/webhooks.html#secret-token.
"""
token = self.request.META.get(GITLAB_TOKEN_HEADER)
token = self.request.headers.get(GITLAB_TOKEN_HEADER, "")
secret = self.get_integration().secret
if not secret:
log.debug('Skipping payload signature validation.')
return True
if not token:
return False
return token == secret
return constant_time_compare(secret, token)

def get_external_version_data(self):
"""Get commit SHA and merge request number from payload."""
Expand Down Expand Up @@ -597,6 +593,8 @@ def handle_webhook(self):
event=event,
)

integration = self.get_integration()

# Always update `latest` branch to point to the default branch in the repository
# even if the event is not gonna be handled. This helps us to keep our db in sync.
default_branch = self.data.get("project", {}).get("default_branch", None)
Expand All @@ -623,15 +621,16 @@ def handle_webhook(self):
except KeyError as exc:
raise ParseError('Parameter "ref" is required') from exc

if self.project.external_builds_enabled and event == GITLAB_MERGE_REQUEST:
if (
action in
[
GITLAB_MERGE_REQUEST_OPEN,
GITLAB_MERGE_REQUEST_REOPEN,
GITLAB_MERGE_REQUEST_UPDATE
]
):
if (
integration.secret
and self.project.external_builds_enabled
and event == GITLAB_MERGE_REQUEST
):
if action in [
GITLAB_MERGE_REQUEST_OPEN,
GITLAB_MERGE_REQUEST_REOPEN,
GITLAB_MERGE_REQUEST_UPDATE,
]:
# Handle open, update, reopen merge_request event.
return self.get_external_version_response(self.project)

Expand Down Expand Up @@ -687,7 +686,7 @@ def handle_webhook(self):
it sets the new attribute (null if it is a deletion) and the old
attribute (null if it is a creation).
"""
event = self.request.META.get(BITBUCKET_EVENT_HEADER, BITBUCKET_PUSH)
event = self.request.headers.get(BITBUCKET_EVENT_HEADER, BITBUCKET_PUSH)
log.bind(webhook_event=event)
webhook_bitbucket.send(
Project,
Expand Down Expand Up @@ -727,8 +726,27 @@ def handle_webhook(self):
return None

def is_payload_valid(self):
"""Bitbucket doesn't have an option for payload validation."""
return True
"""
BitBucket use a HMAC hexdigest hash to sign the payload.
It is sent in the request's header.
See https://support.atlassian.com/bitbucket-cloud/docs/manage-webhooks/#Secure-webhooks.
"""
signature = self.request.headers.get(BITBUCKET_SIGNATURE_HEADER)
secret = self.get_integration().secret
if not secret:
log.debug("Skipping payload signature validation.")
return True
if not signature:
return False
msg = self.request.body.decode()
digest = WebhookMixin.get_digest(secret, msg)
result = hmac.compare_digest(
b"sha256=" + digest.encode(),
signature.encode(),
)
return result


class IsAuthenticatedOrHasToken(permissions.IsAuthenticated):
Expand Down Expand Up @@ -783,9 +801,8 @@ def get_project(self, **kwargs):
if token:
integration = self.get_integration()
obj = Project.objects.get(**kwargs)
is_valid = (
integration.project == obj and
token == getattr(integration, 'token', None)
is_valid = integration.project == obj and constant_time_compare(
token, getattr(integration, "token", "")
)
if is_valid:
return obj
Expand All @@ -810,10 +827,10 @@ def handle_webhook(self):

def is_payload_valid(self):
"""
We can't have payload validation in the generic webhook.
Generic webhooks don't have payload validation.
Since we don't know the system that would trigger the webhook.
We have a token for authentication.
We use basic auth or token auth to validate that the user has access to
the project and integration (get_project() method).
"""
return True

Expand Down
3 changes: 0 additions & 3 deletions readthedocs/integrations/migrations/0005_change_default_integration_secret.py
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# Generated by Django 1.11.16 on 2018-12-10 22:08
from django.db import migrations, models

import readthedocs.integrations.utils


class Migration(migrations.Migration):
dependencies = [
Expand All @@ -15,7 +13,6 @@ class Migration(migrations.Migration):
name="secret",
field=models.CharField(
blank=True,
default=readthedocs.integrations.utils.get_secret,
help_text="Secret used to validate the payload of the webhook",
max_length=255,
null=True,
Expand Down
Loading

0 comments on commit 55511b4

Please sign in to comment.