From 843d7c71926b03f0c5847a0740ac3f7fb3c5fbad Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Thu, 4 Apr 2024 09:33:35 +0200 Subject: [PATCH] fix(security): validate GitHub user during integration installation (#67876) We're adding one more step in the GitHub integration installation pipeline, namely GitHub OAuth2 authorize. This is transparent from the UX perspective as the data exchange happens without user interaction. The pipeline will now fail in these cases: - If there is a mismatch between currently authenticated GitHub user (derived from OAuth2 authorize step) and the user who installed the GitHub app (https://github.com/apps/sentry-io) - If there is a mismatch between `state` parameter supplied by user and pipeline signature - If GitHub could not generate correct `access_token` from the `code` (wrong or attempt of re-use of `code`). In all those cases, this error is shown: ![image](https://github.com/getsentry/sentry/assets/1127549/18923861-2ead-4cf5-adda-7738aef801d7) --- src/sentry/integrations/github/integration.py | 159 +++++++++++++----- src/sentry/web/frontend/pipeline_advancer.py | 11 +- .../integrations/github/test_integration.py | 127 +++++++++++++- 3 files changed, 247 insertions(+), 50 deletions(-) diff --git a/src/sentry/integrations/github/integration.py b/src/sentry/integrations/github/integration.py index 2e157a08961..fa535e4bd4a 100644 --- a/src/sentry/integrations/github/integration.py +++ b/src/sentry/integrations/github/integration.py @@ -4,8 +4,10 @@ import re from collections.abc import Collection, Mapping, Sequence from typing import Any +from urllib.parse import parse_qsl from django.http import HttpResponse +from django.urls import reverse from django.utils.text import slugify from django.utils.translation import gettext_lazy as _ from rest_framework.request import Request @@ -13,6 +15,8 @@ from sentry import features, options from sentry.api.utils import generate_organization_url from sentry.constants import ObjectStatus +from sentry.http import safe_urlopen, safe_urlread +from sentry.identity.github import GitHubIdentityProvider, get_user_info from sentry.integrations import ( FeatureDescription, IntegrationFeatures, @@ -35,6 +39,7 @@ from sentry.tasks.integrations.github.constants import RATE_LIMITED_MESSAGE from sentry.tasks.integrations.link_all_repos import link_all_repos from sentry.utils import metrics +from sentry.utils.http import absolute_uri from sentry.web.helpers import render_to_response from .client import GitHubAppsClient, GitHubClientMixin @@ -108,6 +113,9 @@ ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG = _( "It seems that your GitHub account has been installed on another Sentry organization. Please uninstall and try again." ) +ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST = _( + "We could not verify the authenticity of the installation request. We recommend restarting the installation process." +) ERR_INTEGRATION_PENDING_DELETION = _( "It seems that your Sentry organization has an installation pending deletion. Please wait ~15min for the uninstall to complete and try again." ) @@ -118,6 +126,32 @@ def build_repository_query(metadata: Mapping[str, Any], name: str, query: str) - return f"{account_type}:{name} {query}".encode() +def error( + request, + org, + error_short="Invalid installation request.", + error_long=ERR_INTEGRATION_INVALID_INSTALLATION_REQUEST, +): + return render_to_response( + "sentry/integrations/github-integration-failed.html", + context={ + "error": error_long, + "payload": { + "success": False, + "data": {"error": _(error_short)}, + }, + "document_origin": get_document_origin(org), + }, + request=request, + ) + + +def get_document_origin(org) -> str: + if org and features.has("organizations:customer-domains", org.organization): + return f'"{generate_organization_url(org.organization.slug)}"' + return "document.origin" + + # Github App docs and list of available endpoints # https://docs.github.com/en/rest/apps/installations # https://docs.github.com/en/rest/overview/endpoints-available-for-github-apps @@ -307,7 +341,7 @@ def post_install( ) def get_pipeline_views(self) -> Sequence[PipelineView]: - return [GitHubInstallation()] + return [OAuthLoginView(), GitHubInstallation()] def get_installation_info(self, installation_id: str) -> Mapping[str, Any]: client = self.get_client() @@ -352,15 +386,72 @@ def setup(self) -> None: ) +class OAuthLoginView(PipelineView): + def dispatch(self, request: Request, pipeline) -> HttpResponse: + self.determine_active_organization(request) + + ghip = GitHubIdentityProvider() + github_client_id = ghip.get_oauth_client_id() + github_client_secret = ghip.get_oauth_client_secret() + + installation_id = request.GET.get("installation_id") + if installation_id: + pipeline.bind_state("installation_id", installation_id) + + if not request.GET.get("state"): + state = pipeline.signature + + redirect_uri = absolute_uri( + reverse("sentry-extension-setup", kwargs={"provider_id": "github"}) + ) + return self.redirect( + f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" + ) + + # At this point, we are past the GitHub "authorize" step + if request.GET.get("state") != pipeline.signature: + return error(request, self.active_organization) + + # similar to OAuth2CallbackView.get_token_params + data = { + "code": request.GET.get("code"), + "client_id": github_client_id, + "client_secret": github_client_secret, + } + + # similar to OAuth2CallbackView.exchange_token + req = safe_urlopen(url=ghip.get_oauth_access_token_url(), data=data) + + try: + body = safe_urlread(req).decode("utf-8") + payload = dict(parse_qsl(body)) + except Exception: + payload = {} + + if "access_token" not in payload: + return error(request, self.active_organization) + + authenticated_user_info = get_user_info(payload["access_token"]) + if "login" not in authenticated_user_info: + return error(request, self.active_organization) + + pipeline.bind_state("github_authenticated_user", authenticated_user_info["login"]) + return pipeline.next_step() + + class GitHubInstallation(PipelineView): def get_app_url(self) -> str: name = options.get("github-app.name") return f"https://github.com/apps/{slugify(name)}" def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse: - if "installation_id" not in request.GET: + installation_id = request.GET.get( + "installation_id", pipeline.fetch_state("installation_id") + ) + if installation_id is None: return self.redirect(self.get_app_url()) + pipeline.bind_state("installation_id", installation_id) self.determine_active_organization(request) integration_pending_deletion_exists = False @@ -374,57 +465,43 @@ def dispatch(self, request: Request, pipeline: Pipeline) -> HttpResponse: ).exists() if integration_pending_deletion_exists: - document_origin = "document.origin" - if self.active_organization and features.has( - "organizations:customer-domains", self.active_organization.organization - ): - document_origin = ( - f'"{generate_organization_url(self.active_organization.organization.slug)}"' - ) - return render_to_response( - "sentry/integrations/github-integration-failed.html", - context={ - "error": ERR_INTEGRATION_PENDING_DELETION, - "payload": { - "success": False, - "data": {"error": _("GitHub installation pending deletion.")}, - }, - "document_origin": document_origin, - }, - request=request, + return error( + request, + self.active_organization, + error_short="GitHub installation pending deletion.", + error_long=ERR_INTEGRATION_PENDING_DELETION, ) try: # We want to limit GitHub integrations to 1 organization installations_exist = OrganizationIntegration.objects.filter( - integration=Integration.objects.get(external_id=request.GET["installation_id"]) + integration=Integration.objects.get(external_id=installation_id) ).exists() except Integration.DoesNotExist: - pipeline.bind_state("installation_id", request.GET["installation_id"]) return pipeline.next_step() if installations_exist: - document_origin = "document.origin" - if self.active_organization and features.has( - "organizations:customer-domains", self.active_organization.organization - ): - document_origin = ( - f'"{generate_organization_url(self.active_organization.organization.slug)}"' - ) - return render_to_response( - "sentry/integrations/github-integration-failed.html", - context={ - "error": ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, - "payload": { - "success": False, - "data": {"error": _("Github installed on another Sentry organization.")}, - }, - "document_origin": document_origin, - }, - request=request, + return error( + request, + self.active_organization, + error_short="Github installed on another Sentry organization.", + error_long=ERR_INTEGRATION_EXISTS_ON_ANOTHER_ORG, ) # OrganizationIntegration does not exist, but Integration does exist. - pipeline.bind_state("installation_id", request.GET["installation_id"]) + try: + integration = Integration.objects.get( + external_id=installation_id, status=ObjectStatus.ACTIVE + ) + except Integration.DoesNotExist: + return error(request, self.active_organization) + + # Check that the authenticated GitHub user is the same as who installed the app. + if ( + pipeline.fetch_state("github_authenticated_user") + != integration.metadata["sender"]["login"] + ): + return error(request, self.active_organization) + return pipeline.next_step() diff --git a/src/sentry/web/frontend/pipeline_advancer.py b/src/sentry/web/frontend/pipeline_advancer.py index 63183a4d271..cfe97929605 100644 --- a/src/sentry/web/frontend/pipeline_advancer.py +++ b/src/sentry/web/frontend/pipeline_advancer.py @@ -16,12 +16,6 @@ PIPELINE_CLASSES = [IntegrationPipeline, IdentityProviderPipeline] -# GitHub apps may be installed directly from GitHub, in which case -# they will redirect here *without* being in the pipeline. If that happens -# redirect to the integration install org picker. -FORWARD_INSTALL_FOR = ["github"] - - from rest_framework.request import Request @@ -40,8 +34,11 @@ def handle(self, request: Request, provider_id: str) -> HttpResponseBase: if pipeline: break + # GitHub apps may be installed directly from GitHub, in which case + # they will redirect here *without* being in the pipeline. If that happens + # redirect to the integration install org picker. if ( - provider_id in FORWARD_INSTALL_FOR + provider_id == "github" and request.GET.get("setup_action") == "install" and pipeline is None ): diff --git a/tests/sentry/integrations/github/test_integration.py b/tests/sentry/integrations/github/test_integration.py index c6417da8ab9..895219d1706 100644 --- a/tests/sentry/integrations/github/test_integration.py +++ b/tests/sentry/integrations/github/test_integration.py @@ -12,6 +12,7 @@ from django.urls import reverse import sentry +from fixtures.github import INSTALLATION_EVENT_EXAMPLE from sentry.api.utils import generate_organization_url from sentry.constants import ObjectStatus from sentry.integrations.github import ( @@ -32,6 +33,7 @@ from sentry.silo.base import SiloMode from sentry.testutils.cases import IntegrationTestCase from sentry.testutils.silo import assume_test_silo_mode, control_silo_test +from sentry.utils import json from sentry.utils.cache import cache TREE_RESPONSES = { @@ -111,6 +113,15 @@ def _stub_github(self): self.gh_org = "Test-Organization" pp = 1 + access_token = "xxxxx-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx" + responses.add( + responses.POST, + "https://github.com/login/oauth/access_token", + body=f"access_token={access_token}", + ) + + responses.add(responses.GET, self.base_url + "/user", json={"login": "octocat"}) + responses.add( responses.POST, self.base_url + f"/app/installations/{self.installation_id}/access_tokens", @@ -218,6 +229,24 @@ def assert_setup_flow(self): redirect = urlparse(resp["Location"]) assert redirect.scheme == "https" assert redirect.netloc == "github.com" + assert redirect.path == "/login/oauth/authorize" + assert ( + redirect.query + == "client_id=github-client-id&state=9cae5e88803f35ed7970fc131e6e65d3&redirect_uri=http://testserver/extensions/github/setup/" + ) + + resp = self.client.get( + "{}?{}".format( + self.setup_path, + urlencode( + {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"} + ), + ) + ) + assert resp.status_code == 302 + redirect = urlparse(resp["Location"]) + assert redirect.scheme == "https" + assert redirect.netloc == "github.com" assert redirect.path == "/apps/sentry-test-app" # App installation ID is provided @@ -225,7 +254,7 @@ def assert_setup_flow(self): "{}?{}".format(self.setup_path, urlencode({"installation_id": self.installation_id})) ) - auth_header = responses.calls[0].request.headers["Authorization"] + auth_header = responses.calls[2].request.headers["Authorization"] assert auth_header == "Bearer jwt_token_1" self.assertDialogSuccess(resp) @@ -303,8 +332,15 @@ def test_github_installed_on_another_org(self): ), urlencode({"installation_id": self.installation_id}), ) + self.setup_path_2 = "{}?{}".format( + self.setup_path, + urlencode( + {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"} + ), + ) with self.feature({"organizations:customer-domains": [self.organization_2.slug]}): resp = self.client.get(self.init_path_2) + resp = self.client.get(self.setup_path_2) self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html") assert ( b'{"success":false,"data":{"error":"Github installed on another Sentry organization."}}' @@ -330,6 +366,8 @@ def test_github_installed_on_another_org(self): # Try again and should be successful resp = self.client.get(self.init_path_2) + resp = self.client.get(self.setup_path_2) + self.assertDialogSuccess(resp) integration = Integration.objects.get(external_id=self.installation_id) assert integration.provider == "github" @@ -347,7 +385,73 @@ def test_installation_not_found(self): resp = self.client.get( "{}?{}".format(self.setup_path, urlencode({"installation_id": self.installation_id})) ) - assert b"The GitHub installation could not be found." in resp.content + resp = self.client.get( + "{}?{}".format( + self.setup_path, + urlencode( + {"code": "12345678901234567890", "state": "ddd023d87a913d5226e2a882c4c4cc05"} + ), + ) + ) + assert b"Invalid installation request." in resp.content + + @responses.activate + def test_github_user_mismatch(self): + self._stub_github() + + # Emulate GitHub installation + init_path_1 = "{}?{}".format( + reverse( + "sentry-organization-integrations-setup", + kwargs={ + "organization_slug": self.organization.slug, + "provider_id": self.provider.key, + }, + ), + urlencode({"installation_id": self.installation_id}), + ) + self.client.get(init_path_1) + + webhook_event = json.loads(INSTALLATION_EVENT_EXAMPLE) + webhook_event["installation"]["id"] = self.installation_id + webhook_event["sender"]["login"] = "attacker" + resp = self.client.post( + path="/extensions/github/webhook/", + data=json.dumps(webhook_event), + content_type="application/json", + HTTP_X_GITHUB_EVENT="installation", + HTTP_X_HUB_SIGNATURE="sha1=d184e6717f8bfbcc291ebc8c0756ee446c6c9486", + HTTP_X_GITHUB_DELIVERY="00000000-0000-4000-8000-1234567890ab", + ) + assert resp.status_code == 204 + + # Validate the installation user + user_2 = self.create_user("foo@example.com") + org_2 = self.create_organization(name="Rowdy Tiger", owner=user_2) + self.login_as(user_2) + init_path_2 = "{}?{}".format( + reverse( + "sentry-organization-integrations-setup", + kwargs={ + "organization_slug": org_2.slug, + "provider_id": self.provider.key, + }, + ), + urlencode({"installation_id": self.installation_id}), + ) + setup_path_2 = "{}?{}".format( + self.setup_path, + urlencode( + {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"} + ), + ) + with self.feature({"organizations:customer-domains": [org_2.slug]}): + resp = self.client.get(init_path_2) + resp = self.client.get(setup_path_2) + self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html") + assert resp.status_code == 200 + assert b'window.opener.postMessage({"success":false' in resp.content + assert b"Invalid installation request." in resp.content @responses.activate def test_disable_plugin_when_fully_migrated(self): @@ -594,6 +698,17 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self): resp = self.client.get( "{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id})) ) + resp = self.client.get( + "{}?{}".format( + self.setup_path, + urlencode( + { + "code": "12345678901234567890", + "state": "9cae5e88803f35ed7970fc131e6e65d3", + } + ), + ) + ) assert resp.status_code == 200 self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html") @@ -615,6 +730,14 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self): resp = self.client.get( "{}?{}".format(self.init_path, urlencode({"installation_id": self.installation_id})) ) + resp = self.client.get( + "{}?{}".format( + self.setup_path, + urlencode( + {"code": "12345678901234567890", "state": "9cae5e88803f35ed7970fc131e6e65d3"} + ), + ) + ) self.assertDialogSuccess(resp) integration = Integration.objects.get(external_id=self.installation_id) assert integration.provider == "github"