Skip to content

Commit

Permalink
feat(backup): Generate new UUIDs and secrets when needed (#55826)
Browse files Browse the repository at this point in the history
UUIDs should always be unique (it's right in the name!). When we import
a `UUIDField`, we should generate a new UUID for it, and update all
relevant references.

Similarly, we enforce `ProjectKey` API token uniqueness. When importing
a colliding token, which will likely happen, we generate new API keys
for that project. This may break some user flows during cross-region
relocation, so there is a note to revisit this decision later.

Issue: getsentry/team-ospo#193
  • Loading branch information
azaslavsky authored Sep 8, 2023
1 parent ba45f96 commit 3e3a46c
Show file tree
Hide file tree
Showing 7 changed files with 400 additions and 4 deletions.
96 changes: 95 additions & 1 deletion src/sentry/backup/comparators.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import re
from abc import ABC, abstractmethod
from collections import defaultdict
from functools import lru_cache
Expand Down Expand Up @@ -390,6 +391,93 @@ def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Compa
return []


class RegexComparator(JSONScrubbingComparator, ABC):
"""Comparator that ensures that both sides match a certain regex."""

def __init__(self, regex: re.Pattern, *fields: str):
self.regex = regex
super().__init__(*fields)

def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[ComparatorFinding]:
findings = []
fields = sorted(self.fields)
for f in fields:
if left["fields"].get(f) is None and right["fields"].get(f) is None:
continue

lv = left["fields"][f]
if not self.regex.fullmatch(lv):
findings.append(
ComparatorFinding(
kind=self.get_kind(),
on=on,
left_pk=left["pk"],
right_pk=right["pk"],
reason=f"""the left value ("{lv}") of `{f}` was not matched by this regex: {self.regex.pattern}""",
)
)

rv = right["fields"][f]
if not self.regex.fullmatch(rv):
findings.append(
ComparatorFinding(
kind=self.get_kind(),
on=on,
left_pk=left["pk"],
right_pk=right["pk"],
reason=f"""the right value ("{rv}") of `{f}` was not matched by this regex: {self.regex.pattern}""",
)
)
return findings


class SecretHexComparator(RegexComparator):
"""Certain 16-byte hexadecimal API keys are regenerated during an import operation."""

def __init__(self, bytes: int, *fields: str):
super().__init__(re.compile(f"""^[0-9a-f]{{{bytes * 2}}}$"""), *fields)


# Note: we could also use the `uuid` Python uuid module for this, but it is finicky and accepts some
# weird syntactic variations that are not very common and may cause weird failures when they are
# rejected elsewhere.
class UUID4Comparator(RegexComparator):
"""UUIDs must be regenerated on import (otherwise they would not be unique...). This comparator ensures that they retain their validity, but are not equivalent."""

def __init__(self, *fields: str):
super().__init__(
re.compile(
"^[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}\\Z$", re.I
),
*fields,
)

def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[ComparatorFinding]:
# First, ensure that the two sides are not equivalent.
findings = []
fields = sorted(self.fields)
for f in fields:
if left["fields"].get(f) is None and right["fields"].get(f) is None:
continue

lv = left["fields"][f]
rv = right["fields"][f]
if lv == rv:
findings.append(
ComparatorFinding(
kind=self.get_kind(),
on=on,
left_pk=left["pk"],
right_pk=right["pk"],
reason=f"""the left value ({lv}) of the UUID field `{f}` was equal to the right value ({rv})""",
)
)

# Now, make sure both UUIDs are valid.
findings.extend(super().compare(on, left, right))
return findings


def auto_assign_datetime_equality_comparators(comps: ComparatorMap) -> None:
"""Automatically assigns the DateAddedComparator to any `DateTimeField` that is not already claimed by the `DateUpdatedComparator`."""

Expand Down Expand Up @@ -469,15 +557,21 @@ def get_default_comparators():
"sentry.apiapplication": [HashObfuscatingComparator("client_id", "client_secret")],
"sentry.authidentity": [HashObfuscatingComparator("ident", "token")],
"sentry.alertrule": [DateUpdatedComparator("date_modified")],
"sentry.incident": [UUID4Comparator("detection_uuid")],
"sentry.incidentactivity": [UUID4Comparator("notification_uuid")],
"sentry.incidenttrigger": [DateUpdatedComparator("date_modified")],
"sentry.integration": [DateUpdatedComparator("date_updated")],
"sentry.monitor": [UUID4Comparator("guid")],
"sentry.orgauthtoken": [
HashObfuscatingComparator("token_hashed", "token_last_characters")
],
"sentry.organization": [AutoSuffixComparator("slug")],
"sentry.organizationintegration": [DateUpdatedComparator("date_updated")],
"sentry.organizationmember": [HashObfuscatingComparator("token")],
"sentry.projectkey": [HashObfuscatingComparator("public_key", "secret_key")],
"sentry.projectkey": [
HashObfuscatingComparator("public_key", "secret_key"),
SecretHexComparator(16, "public_key", "secret_key"),
],
"sentry.querysubscription": [DateUpdatedComparator("date_updated")],
"sentry.relay": [HashObfuscatingComparator("relay_id", "public_key")],
"sentry.relayusage": [HashObfuscatingComparator("relay_id", "public_key")],
Expand Down
14 changes: 14 additions & 0 deletions src/sentry/backup/findings.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,20 @@ class ComparatorFindingKind(IntEnum):
# `None`.
IgnoredComparatorExistenceCheck = auto()

# Secret token fields did not match their regex specification.
SecretHexComparator = auto()

# Failed to compare a secret token field because one of the fields being compared was not
# present or `None`.
SecretHexComparatorExistenceCheck = auto()

# UUID4 fields did not match their regex specification.
UUID4Comparator = auto()

# Failed to compare a UUID4 field because one of the fields being compared was not present or
# `None`.
UUID4ComparatorExistenceCheck = auto()


class ComparatorFinding(NamedTuple):
"""Store all information about a single failed matching between expected and actual output."""
Expand Down
29 changes: 28 additions & 1 deletion src/sentry/incidents/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@

from collections import namedtuple
from enum import Enum
from typing import Optional
from uuid import uuid4

from django.conf import settings
from django.core.cache import cache
from django.db import IntegrityError, models, router, transaction
from django.db.models.signals import post_delete, post_save
from django.utils import timezone

from sentry.backup.scopes import RelocationScope
from sentry.backup.dependencies import PrimaryKeyMap
from sentry.backup.scopes import ImportScope, RelocationScope
from sentry.db.models import (
ArrayField,
FlexibleForeignKey,
Expand Down Expand Up @@ -212,6 +215,18 @@ def current_end_date(self):
def duration(self):
return self.current_end_date - self.date_started

def _normalize_before_relocation_import(
self, pk_map: PrimaryKeyMap, scope: ImportScope
) -> Optional[int]:
old_pk = super()._normalize_before_relocation_import(pk_map, scope)
if old_pk is None:
return None

# Generate a new UUID, if one exists.
if self.detection_uuid:
self.detection_uuid = uuid4()
return old_pk


@region_silo_only_model
class PendingIncidentSnapshot(Model):
Expand Down Expand Up @@ -280,6 +295,18 @@ class Meta:
app_label = "sentry"
db_table = "sentry_incidentactivity"

def _normalize_before_relocation_import(
self, pk_map: PrimaryKeyMap, scope: ImportScope
) -> Optional[int]:
old_pk = super()._normalize_before_relocation_import(pk_map, scope)
if old_pk is None:
return None

# Generate a new UUID, if one exists.
if self.notification_uuid:
self.notification_uuid = uuid4()
return old_pk


@region_silo_only_model
class IncidentSubscription(Model):
Expand Down
13 changes: 12 additions & 1 deletion src/sentry/models/projectkey.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,18 @@ def write_relocation_import(
if old_pk is None:
return None

(key, _) = self.__class__.objects.get_or_create(
# If there is a key collision, generate new keys.
#
# TODO(getsentry/team-ospo#190): Is this the right behavior? Another option is to just
# update the SQL schema to no longer require uniqueness.
matching_public_key = self.__class__.objects.filter(public_key=self.public_key).first()
if not self.public_key or matching_public_key:
self.public_key = self.generate_api_key()
matching_secret_key = self.__class__.objects.filter(secret_key=self.secret_key).first()
if not self.secret_key or matching_secret_key:
self.secret_key = self.generate_api_key()

(key, _) = ProjectKey.objects.get_or_create(
project=self.project, defaults=model_to_dict(self)
)
if key:
Expand Down
15 changes: 14 additions & 1 deletion src/sentry/monitors/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
from datetime import datetime, timedelta
from typing import TYPE_CHECKING, Any, Dict, Optional
from uuid import uuid4

import jsonschema
import pytz
Expand All @@ -14,7 +15,8 @@
from django.dispatch import receiver
from django.utils import timezone

from sentry.backup.scopes import RelocationScope
from sentry.backup.dependencies import PrimaryKeyMap
from sentry.backup.scopes import ImportScope, RelocationScope
from sentry.constants import ObjectStatus
from sentry.db.models import (
BaseManager,
Expand Down Expand Up @@ -357,6 +359,17 @@ def get_alert_rule_data(self):

return None

def _normalize_before_relocation_import(
self, pk_map: PrimaryKeyMap, scope: ImportScope
) -> Optional[int]:
old_pk = super()._normalize_before_relocation_import(pk_map, scope)
if old_pk is None:
return None

# Generate a new UUID.
self.guid = uuid4()
return old_pk


@receiver(pre_save, sender=Monitor)
def check_organization_monitor_limits(sender, instance, **kwargs):
Expand Down
Loading

0 comments on commit 3e3a46c

Please sign in to comment.