From 07bc51b890d547252f347876619b0055844e18df Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 15 Oct 2026 18:08:11 -0400 Subject: [PATCH] feat(523): forge-connection tokens are stored sealed under SECRET_KEY, like the backup S3 secret The column becomes token_sealed (migration 0122 seals existing rows with a frozen copy of the v1 key), every reader opens it through connection_token, a row this instance cannot open is skipped with a warning, and Settings asks for that token again. Co-Authored-By: Claude Opus 5.5 --- alembic/versions/0122_seal_forge_tokens.py | 82 ++++++++++++++++++++++ docs/backups.md | 4 +- docs/configuration.md | 3 +- frontend/src/views/SettingsView.vue | 24 ++++++- src/scribe/models/forge_connection.py | 6 +- src/scribe/routes/settings.py | 19 +++-- src/scribe/services/forge.py | 20 +++++- src/scribe/services/forge_connections.py | 17 ++++- tests/test_forge_token_sealing.py | 47 +++++++++++++ tests/test_integration_forge_keyring.py | 26 +++++++ 10 files changed, 232 insertions(+), 16 deletions(-) create mode 100644 alembic/versions/0122_seal_forge_tokens.py create mode 100644 tests/test_forge_token_sealing.py diff --git a/alembic/versions/0122_seal_forge_tokens.py b/alembic/versions/0122_seal_forge_tokens.py new file mode 100644 index 00000000..067f41e1 --- /dev/null +++ b/alembic/versions/0122_seal_forge_tokens.py @@ -0,0 +1,82 @@ +"""Seal forge-connection tokens at rest (milestone 523) + +Revision ID: 0122 +Revises: 0121 +Create Date: 2026-10-15 + +A forge token was stored as plain text, so a copy of the database (or of a +backup tool's snapshot of it) was enough to read every user's forge +credential. Tokens are now sealed the way the backup destination's S3 secret +is (services/secrets.py): Fernet under a key derived from SECRET_KEY, which +lives in the deployment's environment rather than in the data. + +The column is renamed `token` -> `token_sealed` in the same step. The name +says what the bytes are, and every reader has to change with it, so a reader +missed here fails on a missing attribute instead of quietly sending +ciphertext to the forge as a token. + +THE KEY DERIVATION IS FROZEN HERE, not imported. This revision must seal with +exactly v1's key forever; importing services.secrets would let a later edit +there change what an old migration writes. The unit lane checks that what +this produces is what `secrets.unseal` opens today. + +Cost: one row per (user, forge host) — a handful per instance. + +Downgrade opens each token back into plain text. A row sealed under a +different SECRET_KEY cannot be opened and comes back empty; its owner enters +the token again. +""" +import base64 +import hashlib + +import sqlalchemy as sa +from alembic import op + +revision = "0122" +down_revision = "0121" +branch_labels = None +depends_on = None + +_LABEL_V1 = b"scribe.secrets.v1\x00" + + +def _fernet(): + from cryptography.fernet import Fernet + + from scribe.config import Config + + digest = hashlib.sha256(_LABEL_V1 + Config.SECRET_KEY.encode()).digest() + return Fernet(base64.urlsafe_b64encode(digest)) + + +def seal_v1(plain: str) -> str: + return _fernet().encrypt(plain.encode()).decode() + + +def upgrade() -> None: + op.alter_column("forge_connections", "token", new_column_name="token_sealed") + bind = op.get_bind() + rows = bind.execute(sa.text("SELECT id, token_sealed FROM forge_connections")).all() + for row_id, plain in rows: + bind.execute( + sa.text("UPDATE forge_connections SET token_sealed = :v WHERE id = :id"), + {"v": seal_v1(plain or ""), "id": row_id}, + ) + + +def downgrade() -> None: + from cryptography.fernet import InvalidToken + + bind = op.get_bind() + fernet = _fernet() + rows = bind.execute(sa.text("SELECT id, token_sealed FROM forge_connections")).all() + for row_id, sealed in rows: + try: + plain = fernet.decrypt((sealed or "").encode()).decode() + except (InvalidToken, ValueError): + plain = "" + bind.execute( + sa.text("UPDATE forge_connections SET token_sealed = :v WHERE id = :id"), + {"v": plain, "id": row_id}, + ) + op.alter_column("forge_connections", "token_sealed", new_column_name="token") diff --git a/docs/backups.md b/docs/backups.md index 2adc52c9..2fe45b6b 100644 --- a/docs/backups.md +++ b/docs/backups.md @@ -26,7 +26,9 @@ differently. It does **not** hold: - **Search indexes.** They are rebuilt from the records when Scribe starts. - **Sharing:** groups, project shares and note shares. - **Credentials:** API keys and forge connections. After a restore, issue new - API keys and add forge connections again. + API keys and add forge connections again. (Forge tokens are also sealed under + `SECRET_KEY` in the database itself, so a copy of the database doesn't carry + them in readable form either.) - **Logs and notifications.** - **The backup destination's own S3 secret.** diff --git a/docs/configuration.md b/docs/configuration.md index e77c1452..1bd69907 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -9,7 +9,7 @@ Configuration is via environment variables. The `docker-compose.yml` file sets d | Variable | Default | Description | |----------|---------|-------------| | `DATABASE_URL` | `postgresql+asyncpg://fabled:fabled@db/scribe` | PostgreSQL async connection string | -| `SECRET_KEY` | `dev-secret-change-me` | Session signing key — **change this in production** | +| `SECRET_KEY` | `dev-secret-change-me` | Session signing key, and the key stored credentials are sealed under — **change this in production, then keep it** | | `SECRET_KEY_FILE` | — | Path to a Docker secret file containing the key (alternative to `SECRET_KEY`) | | `LOG_LEVEL` | `INFO` | Logging verbosity (`DEBUG`, `INFO`, `WARNING`, `ERROR`) | | `SECURE_COOKIES` | `false` | Set `true` when running behind TLS | @@ -127,6 +127,7 @@ If using `TRUST_PROXY_HEADERS=true`, ensure your proxy strips any client-supplie python3 -c "import secrets; print(secrets.token_hex(32))" ``` Or use Docker Secrets via `SECRET_KEY_FILE`. +- **Keep `SECRET_KEY` once it is set.** Scribe seals the credentials it stores under a key derived from it: forge-connection tokens and the backup destination's S3 secret. A copy of the database alone can't read them. If the key changes (for example, a restore onto an instance with a different key), those credentials can no longer be opened. Settings marks each one and asks for it again, and nothing else breaks. - **`SECURE_COOKIES=true`** — must be set when running behind TLS. - **`BASE_URL`** — set to your public URL; required for OIDC redirects and email links. - **Registration** — auto-closes after the first user (admin). Re-enable from Settings → Users or send invite links. diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 4cc4832e..17854217 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -825,14 +825,17 @@ const baseUrlSaved = ref(false); // Git forge connections (#2778) — the user's keyring: one read-only // credential per forge host, used server-side for every forge read on // projects this user owns. The token round-trips masked; the server treats -// the mask as "unchanged". +// the mask as "unchanged". Tokens are stored sealed under SECRET_KEY: +// `token_readable` false means this instance can no longer open one (the key +// changed since it was saved), so the row asks for it again. interface ForgeConnectionEntry { - id: number; kind: string; base_url: string; host: string; + id: number; kind: string; base_url: string; host: string; token_readable: boolean; } const forgeConnections = ref([]); const forgeKinds = ref(["gitea", "github"]); const connForm = ref({ id: 0, kind: "gitea", base_url: "", token: "" }); const connFormOpen = ref(false); +const unreadableConnections = computed(() => forgeConnections.value.filter((c) => !c.token_readable)); const savingConn = ref(false); const testingConnId = ref(0); const connTestResult = ref<{ id: number; ok: boolean; message: string } | null>(null); @@ -2564,13 +2567,23 @@ async function deleteUser(userId: number) { Read-only, base URL https://github.com (or your GitHub Enterprise URL).

+ - +
HostKindBase URL
{{ c.host }} + {{ c.host }} + token needs re-entering + {{ c.kind }} {{ c.base_url }} @@ -4593,6 +4606,11 @@ async function deleteUser(userId: number) { border-radius: var(--fs-radius-md); background: color-mix(in srgb, var(--fs-warning) 12%, var(--fs-surface-raised)); } +.conn-token-lost { + margin-left: var(--fs-space-2); + font-size: var(--fs-size-body-sm); + color: var(--fs-warning-fg); +} .calibration-warning-title { margin: 0 0 var(--fs-space-2); font-size: 0.95rem; diff --git a/src/scribe/models/forge_connection.py b/src/scribe/models/forge_connection.py index 9ecbdeed..d922456b 100644 --- a/src/scribe/models/forge_connection.py +++ b/src/scribe/models/forge_connection.py @@ -16,7 +16,9 @@ class ForgeConnection(Base, TimestampMixin): `host` is derived from `base_url` at write time and stored because it is the lookup key; the service layer keeps the two in step. The token is a - secret: to_dict never includes it, and no route may return it. + secret: it is stored sealed (services/secrets.py — read it through + services.forge_connections.connection_token), to_dict never includes it, + and no route may return it. """ __tablename__ = "forge_connections" @@ -31,7 +33,7 @@ class ForgeConnection(Base, TimestampMixin): kind: Mapped[str] = mapped_column(Text, nullable=False) base_url: Mapped[str] = mapped_column(Text, nullable=False) host: Mapped[str] = mapped_column(Text, nullable=False) - token: Mapped[str] = mapped_column(Text, nullable=False) + token_sealed: Mapped[str] = mapped_column(Text, nullable=False) def to_dict(self) -> dict: return { diff --git a/src/scribe/routes/settings.py b/src/scribe/routes/settings.py index c5e2f3f9..47ba0d1d 100644 --- a/src/scribe/routes/settings.py +++ b/src/scribe/routes/settings.py @@ -125,12 +125,17 @@ async def test_search(): @login_required async def list_forge_connections_route(): from scribe.services.forge import FORGE_KINDS - from scribe.services.forge_connections import list_connections + from scribe.services.forge_connections import connection_token, list_connections uid = get_current_user_id() rows = await list_connections(uid) + # Never the token: only whether this instance can still open it, so the + # page can ask for it again after a SECRET_KEY change. return jsonify({ - "connections": [r.to_dict() for r in rows], + "connections": [ + {**r.to_dict(), "token_readable": connection_token(r) is not None} + for r in rows + ], "kinds": list(FORGE_KINDS), }) @@ -198,13 +203,19 @@ async def test_forge_connection_route(connection_id: int): press, so a misconfiguration is visible now rather than as silent fallbacks later (#2663's lesson, applied per keyring row).""" from scribe.services.forge import ForgeError, build_adapter - from scribe.services.forge_connections import get_connection + from scribe.services.forge_connections import connection_token, get_connection uid = get_current_user_id() row = await get_connection(uid, connection_id) if row is None: return jsonify({"error": "Connection not found"}), 404 - adapter = build_adapter(row.kind, row.base_url, row.token) + token = connection_token(row) + if token is None: + return jsonify({ + "error": "This instance can't read the saved token: SECRET_KEY has changed " + "since it was saved. Enter the token again." + }), 400 + adapter = build_adapter(row.kind, row.base_url, token) if adapter is None: return jsonify({"error": "Connection is not usable — check kind and base URL"}), 400 try: diff --git a/src/scribe/services/forge.py b/src/scribe/services/forge.py index 97119aae..c22b96b2 100644 --- a/src/scribe/services/forge.py +++ b/src/scribe/services/forge.py @@ -358,6 +358,22 @@ _FORGE_CLASSES: dict[str, type[ForgeAdapter]] = { } +def _token_of(row) -> str: + """A keyring row's token, opened; empty when this instance can't open it, + which build_adapter reads as "this connection cannot serve reads".""" + from scribe.services.forge_connections import connection_token + + token = connection_token(row) + if token is None: + logger.warning( + "forge connection %s (%s) holds a token this instance can't open — " + "SECRET_KEY changed since it was saved; its owner must enter it again", + row.id, row.host, + ) + return "" + return token + + def build_adapter( kind: str, base_url: str, token: str, *, transport=None ) -> ForgeAdapter | None: @@ -459,7 +475,7 @@ async def get_forges( if pinned_id: pin = next((r for r in rows if r.id == pinned_id), None) if pin is not None: - adapter = build_adapter(pin.kind, pin.base_url, pin.token, transport=transport) + adapter = build_adapter(pin.kind, pin.base_url, _token_of(pin), transport=transport) return ForgeSelector((adapter,) if adapter is not None else ()) logger.warning( "project %s pins forge connection %s the owner (%s) does not hold — pin ignored", @@ -468,7 +484,7 @@ async def get_forges( adapters: list[ForgeAdapter] = [] for row in rows: - adapter = build_adapter(row.kind, row.base_url, row.token, transport=transport) + adapter = build_adapter(row.kind, row.base_url, _token_of(row), transport=transport) if adapter is not None: adapters.append(adapter) if role == "admin": diff --git a/src/scribe/services/forge_connections.py b/src/scribe/services/forge_connections.py index f18b59c5..9fdf633f 100644 --- a/src/scribe/services/forge_connections.py +++ b/src/scribe/services/forge_connections.py @@ -4,7 +4,9 @@ A connection is a user's read-only credential for one forge host; one row per (user, host) keeps host-keyed resolution deterministic with no default-pointer machinery. Everything here is own-rows-only: a connection is a credential, and no caller — admin included — reads or edits another user's. The token never -leaves the server (model.to_dict omits it; routes mask "set/unset"). +leaves the server (model.to_dict omits it; routes mask "set/unset"), and it is +stored sealed (services/secrets.py): a copy of the database alone cannot read +it. Every reader opens it through `connection_token`. Validation matches what build_adapter will accept, checked here so a bad value is a 400 at the form instead of a silently dead keyring row. @@ -17,6 +19,14 @@ from scribe.models import async_session from scribe.models.forge_connection import ForgeConnection from scribe.models.project import Project from scribe.services.forge import FORGE_KINDS, host_of +from scribe.services.secrets import seal, unseal + + +def connection_token(row: ForgeConnection) -> str | None: + """The connection's token in the clear, or None when this instance can't + open it — sealed under a different SECRET_KEY (a restore onto a new + instance, a rotated key). The owner enters it again; nothing else can.""" + return unseal(row.token_sealed) def validate_connection(kind: str, base_url: str) -> str | None: @@ -82,7 +92,8 @@ async def create_connection( "resolution is by host, so a second row could never be reached" ) row = ForgeConnection( - user_id=user_id, kind=kind, base_url=base_url, host=host, token=token + user_id=user_id, kind=kind, base_url=base_url, host=host, + token_sealed=seal(token), ) session.add(row) await session.commit() @@ -135,7 +146,7 @@ async def update_connection( row.base_url = new_base row.host = new_host if token: - row.token = token + row.token_sealed = seal(token) await session.commit() await session.refresh(row) return row diff --git a/tests/test_forge_token_sealing.py b/tests/test_forge_token_sealing.py new file mode 100644 index 00000000..41744c79 --- /dev/null +++ b/tests/test_forge_token_sealing.py @@ -0,0 +1,47 @@ +"""Forge-connection tokens are stored sealed (milestone 523, #5877). + +Migration 0122 seals the rows that existed before, with its OWN frozen copy of +the v1 key derivation (so a later edit to services/secrets.py can never change +what an old migration writes). That copy is only right while it agrees with +what the app opens today — which is what this pins. If secrets.py ever moves +to a v2 key, it must still open v1 values, and this test says so. +""" +import importlib.util +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +from scribe.services import secrets +from scribe.services.forge_connections import connection_token + +_MIGRATION = Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0122_seal_forge_tokens.py" + + +def _migration(): + spec = importlib.util.spec_from_file_location("migration_0122", _MIGRATION) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_what_the_migration_seals_the_app_opens(): + sealed = _migration().seal_v1("gitea-read-token") + assert "gitea-read-token" not in sealed + assert secrets.unseal(sealed) == "gitea-read-token" + + +def test_a_row_reads_through_connection_token(): + row = SimpleNamespace(token_sealed=secrets.seal("ghp_abc")) + assert connection_token(row) == "ghp_abc" + + +def test_a_row_sealed_under_another_key_reads_as_none(): + row = SimpleNamespace(token_sealed=secrets.seal("ghp_abc")) + with patch.object(secrets.Config, "SECRET_KEY", "a-different-instance"): + assert connection_token(row) is None + + +def test_a_left_over_plain_text_token_is_not_used_as_one(): + """A value that never went through seal is refused, not passed to the + forge as if it were the token: a reader sees None, never the raw bytes.""" + assert connection_token(SimpleNamespace(token_sealed="plain-text-token")) is None diff --git a/tests/test_integration_forge_keyring.py b/tests/test_integration_forge_keyring.py index 27f142ec..0bf28ee5 100644 --- a/tests/test_integration_forge_keyring.py +++ b/tests/test_integration_forge_keyring.py @@ -169,3 +169,29 @@ async def test_env_config_serves_admins_only_and_rows_beat_it(seeded): assert len(selector.adapters) == 1 assert selector.adapters[0]._token == "row-tok" await delete_connection(admin, row.id) + + +@pytest.mark.integration +async def test_the_token_is_stored_sealed_and_a_key_change_drops_the_row(seeded): + """What a copy of the database holds is ciphertext; the reads still get + the token; and a row this instance can no longer open serves nothing + rather than sending ciphertext to the forge as a token.""" + from sqlalchemy import text + + from scribe.services import secrets + + owner = seeded["owner"] + row = await create_connection(owner, kind="gitea", base_url=GITEA, token="plain-tok") + await update_connection(owner, row.id, token="rotated-tok") + async with async_session() as s: + stored = (await s.execute( + text("SELECT token_sealed FROM forge_connections WHERE id = :id"), {"id": row.id} + )).scalar_one() + assert "rotated-tok" not in stored + assert secrets.unseal(stored) == "rotated-tok" + + selector = await get_forges(owner, seeded["project"]) + assert [a._token for a in selector.adapters] == ["rotated-tok"] + + with patch.object(secrets.Config, "SECRET_KEY", "a-different-instance"): + assert not (await get_forges(owner, seeded["project"])).configured