fix(forgejo): deny authorized-integration issuer fetches, and audit every service account #348

Sammanfogat

supernaut sammanfogade 2 incheckningar från fix/integrations-issuer-and-token-inventory in i main

2026-08-02 18:37:45 +00:00

supernaut kommenterad

2026-08-02 15:56:16 +00:00

Ägare

Closes #313, closes #314.

#313 — authorized integrations accepted any issuer domain.

The trust boundary is an outbound fetch on demand: with open registration, any signed-in account
could save an integration naming an arbitrary issuer, and the instance would dial it on save and on
every auth attempt. There is no way in 16.0.1 to disable the feature or withhold it from ordinary
users, so the dial is the only chokepoint — and because it is the whole capability, blocking it
closes the hole rather than narrowing it.

Semantics were read from the pinned source rather than assumed, and the default turned out to be the
bug: an empty allow-list is not "deny" — upstream appends a builtin matcher permitting every
public host. The block-list also beats the allow-list, returning a blocked error even when the
allow-list matched, so * is a genuine master switch.

Now configured deny-all with a second fail-closed gate, so relaxing either one alone still denies.

This settles the question the issue was gated on: the local Actions issuer is exempt from both
checks — validation skips the external-issuer path for the internal placeholder and verifies against
the in-memory key with no HTTP fetch. The existing integration path is unaffected.

Verified empirically on the local preview, with the real refusals:

https://accounts.google.com  → denied by the allow-list check
https://localhost:3000/…     → denied by the block-list check
local Actions integration    → 303, created, unaffected

A trap worth knowing, now documented: the create-authorized-integration admin command bypasses
these settings entirely, because the CLI loads only database settings and never the full
configuration. Testing the fix that way would show it "failing" and is the wrong test.

Also fixed in passing: the local preview's variable file had drifted behind the template it feeds, so
the render — the very verification path the issue names — failed on main. It renders again.

#314 — one admin token was invisible to the audit.

The audit's account list was a hand-kept duplicate of the service-account list, and that duplication
is exactly how the token stayed invisible. It is now derived from the source list, so the two
cannot drift again. Rendered check: 6 → 7 accounts, with the missing one included.

Also documented: the correct scope for that account, established empirically against the preview
(200 on the read, 403 on the write) and confirmed against the source, which keys required scope on
HTTP method; the two package tokens on the CI account with both stores named and the rotation
consequence; a mislabelled comment on the reconciler credential; and the runner-controller's scopes.

Operator actions after merge

Re-mint that account's PAT at the narrower scope — Ansible cannot mint PATs. Exact commands
are in the runbook section this PR adds. A green apply is the proof, since the webhook read fails
on an insufficient scope.
Apply, then run the audit once and confirm seven series and a created-timestamp for the
newly-visible account.
ADR 0024 needs the two missing accounts added and one row corrected — that file lives in
another repository and is outside this change.

Verification

ansible-playbook site.yml --syntax-check clean; ansible-lint 0 failures across 191 files, profile
production; Prettier and markdownlint clean; both templates render and the audit script passes
bash -n.

No --check run, deliberately: one task in this role sets check_mode: false, so a dry run would
reach the live admin API from the control node. Nothing was applied and no production token state was
read.

Closes #313, closes #314. **#313 — authorized integrations accepted any issuer domain.** The trust boundary is an outbound fetch on demand: with open registration, any signed-in account could save an integration naming an arbitrary issuer, and the instance would dial it on save and on every auth attempt. There is no way in 16.0.1 to disable the feature or withhold it from ordinary users, so the dial is the only chokepoint — and because it is the whole capability, blocking it closes the hole rather than narrowing it. Semantics were read from the pinned source rather than assumed, and the default turned out to be the bug: **an empty allow-list is not "deny"** — upstream appends a builtin matcher permitting every public host. The block-list also beats the allow-list, returning a blocked error even when the allow-list matched, so `*` is a genuine master switch. Now configured deny-all with a second fail-closed gate, so relaxing either one alone still denies. **This settles the question the issue was gated on:** the local Actions issuer is exempt from both checks — validation skips the external-issuer path for the internal placeholder and verifies against the in-memory key with no HTTP fetch. The existing integration path is unaffected. Verified empirically on the local preview, with the real refusals: ``` https://accounts.google.com → denied by the allow-list check https://localhost:3000/… → denied by the block-list check local Actions integration → 303, created, unaffected ``` A trap worth knowing, now documented: the `create-authorized-integration` admin command **bypasses these settings entirely**, because the CLI loads only database settings and never the full configuration. Testing the fix that way would show it "failing" and is the wrong test. Also fixed in passing: the local preview's variable file had drifted behind the template it feeds, so the render — the very verification path the issue names — failed on `main`. It renders again. **#314 — one admin token was invisible to the audit.** The audit's account list was a hand-kept duplicate of the service-account list, and that duplication is exactly how the token stayed invisible. It is now **derived** from the source list, so the two cannot drift again. Rendered check: 6 → 7 accounts, with the missing one included. Also documented: the correct scope for that account, established empirically against the preview (200 on the read, 403 on the write) and confirmed against the source, which keys required scope on HTTP method; the two package tokens on the CI account with both stores named and the rotation consequence; a mislabelled comment on the reconciler credential; and the runner-controller's scopes. ### Operator actions after merge 1. **Re-mint that account's PAT at the narrower scope** — Ansible cannot mint PATs. Exact commands are in the runbook section this PR adds. A green apply is the proof, since the webhook read fails on an insufficient scope. 2. **Apply**, then run the audit once and confirm seven series and a created-timestamp for the newly-visible account. 3. **ADR 0024** needs the two missing accounts added and one row corrected — that file lives in another repository and is outside this change. ### Verification `ansible-playbook site.yml --syntax-check` clean; `ansible-lint` 0 failures across 191 files, profile `production`; Prettier and markdownlint clean; both templates render and the audit script passes `bash -n`. No `--check` run, deliberately: one task in this role sets `check_mode: false`, so a dry run would reach the live admin API from the control node. Nothing was applied and no production token state was read.

supernaut lade till 2 incheckningar

2026-08-02 15:56:16 +00:00

fix(forgejo): deny authorized-integration issuer fetches by default b343222953

Authorized Integrations (v16) let ANY signed-in account register a trust
rule naming an OIDC issuer, and Forgejo then fetches that issuer's
discovery document and JWKS — on save and on every authentication
attempt. The routes are under /user/settings, registration is open
(ADR 0029) and app.ini configured none of [authorized_integration], so
"make an account" was enough to have this server emit outbound HTTPS
requests to a host of the visitor's choosing, attributable to our IP.

16.0.1 offers no switch that disables the feature or withholds it from
ordinary users, so the outbound fetch is the only place to stop it — and
it is the whole of the capability, so stopping it closes the finding
rather than narrowing it. We hold no integration today and the only
issuer in the evaluation's GO column is the LOCAL Actions issuer, which
is exempt from both lists (verified in-memory, no HTTP fetch), so the
posture is deny-all rather than a narrower allow-list:

  BLOCKED_DOMAINS = *   master switch; upstream's dialer applies the
                        block list even when the allow list matched, and
                        `*` matches every host and IP
  ALLOWED_DOMAINS       our own domain, as a second fail-closed gate —
                        an EMPTY value is not "deny", upstream appends
                        the builtin `external` matcher to it, which is
                        exactly what this bug was

Relaxing either one alone still leaves the other denying, so a future
PoC has to open this deliberately. ALLOW_LOCALNETWORKS stays false and
REQUEST_TIMEOUT/CACHE_TTL are pinned rather than inherited.

Verified on the local preview (16-rootless, same code as 16.0.1 for
services/auth/authorized_integration.go, modules/hostmatcher/* and
modules/setting/authorized_integration.go): a generic integration on an
outside issuer is refused at the dial, one on an allow-listed host is
refused by the block list, and a forgejo-actions-local integration still
saves. The preview render was broken before this (app.ini.j2 reads
variables vars.local.yml never defined), so local/vars.local.yml gains
the missing ones — that render is the verification path the issue asked
for.

Closes #313

fix(token-audit): audit every provisioned service account, not a hand-kept list

ci / ci (pull_request) Successful in 1m28s

Detaljer

916c37d9e5

gitborg-webhook-admin was in forgejo_service_accounts but not in the
token-audit account list, so its write:admin PAT was the one credential
ForgejoTokenRotationDue could not see. The alert is max by(account) over
the exported creation timestamps: an omitted account does not alert
late, it never alerts, and its token can quietly become the oldest
admin-scoped credential we hold. A credential the audit cannot see is
worse than an old one it can.

So fix the class, not the instance: token_audit_accounts is now derived
from forgejo_service_accounts (with token_audit_accounts_extra for
anything provisioned outside it), which brings the account count from 6
to 7 and makes audited-by-omission impossible for the next account too.

Also reconcile the token inventory with what the repo provisions:

- gitborg-webhook-admin's scope is recorded as read:admin, not
  write:admin. Its only call is GET /api/v1/admin/hooks, and 16.0.1
  derives the required scope level from the HTTP method — verified on
  the local preview: a read:admin token gets 200 on that GET and 403 on
  POST /admin/hooks. The account stays role admin; the route requires
  that of the user, not of the token. The re-mint itself is an operator
  step (Ansible cannot mint PATs) — written up in the runbook.
- gitborg-ci's two write:package tokens are documented as deliberate
  with BOTH stores named (vault, for the host-side mirror and retention;
  org Actions secret REGISTRY_TOKEN, for deploys) and the rotation
  consequence spelled out, since replacing one leaves the other working.
- the reconciler PAT's comment no longer calls it a Kanidm token, and
  the runner-controller scope is named write:admin in both places
  instead of "Actions write scope" in one and write:admin in the other.

ADR 0024's own table lives in the docs repository and still needs the
matching edit: add gitborg-webhook-admin and gitborg-token-audit, and
gitborg-bot's second token.

Closes #314

supernaut tvångsskickade fix/integrations-issuer-and-token-inventory från 916c37d9e5

ci / ci (pull_request) Successful in 1m28s

Detaljer

till 7e213ba775

ci / ci (pull_request) Successful in 2m11s

Detaljer

2026-08-02 18:30:09 +00:00

Jämför

supernaut sammanfogade incheckning 1976ba3db5 till main

2026-08-02 18:37:45 +00:00

supernaut tog bort grenen fix/integrations-issuer-and-token-inventory

2026-08-02 18:37:45 +00:00