Skip to content

🐛 eventing: tenancy key derives from the GitHub login, so a released login inherits the previous holder's tenant #898

Description

@mrsabath

Summary

userkey — the tenancy key that names a user's Kafka topics, their SQLite store and their
ntfy topic — is derived from the GitHub login, which is a renameable display name.
GitHub's durable identifier is the numeric id, and it is already in the response we
parse. Two consequences, one wasteful and one a cross-tenant leak.

Where

eventbridge/ghauth.py:161 reads the login and nothing else:

login = body.get("login")
if not isinstance(login, str) or not login:
    return None, "GitHub returned no login for this token"
return login, None

That string is the userid for the single production derivation point,
eventbridge/auth.py:104:

key = tenancy.userkey(issuer or ISSUER_STATIC, userid)

and shared/tenancy.userkey hashes f"{issuer}\x1f{canon}" where canon is
login.lower() (_canonicalise, github branch).

Failure 1 — a rename moves a user to an empty tenant

$ python3 -c "from shared import tenancy; print(tenancy.userkey('github','alice'))"
gh-alice-ed66acf0
$ python3 -c "from shared import tenancy; print(tenancy.userkey('github','alice-new'))"
gh-alice-new-a4d6e496

Different key, so a different {prefix}-{userkey}-requests topic, a different
sessions.sqlite under the per-user store root, and a different derived ntfy topic. The
user's history does not move with them — it reads as data loss, which is exactly the
symptom DESIGN_PHASE3.md §2.5 and registry.parse's startup refusal were written to
prevent for a derivation change. A rename produces the same outcome through an input
change, which neither guards.

Worth noting what partially saves us today: a user listed in the registry with a recorded
userkey makes this a startup refusal rather than silent breakage
(eventbridge/registry.py:161-171), because the recorded and computed values now
disagree. That is the good failure. But it is a refusal to start, not a migration — and it
only fires for users whose entry records a key. EB_ALLOWED_USERS (Phase 2's path, still
supported) records nothing, so there the rename is silent.

Failure 2 — a released login inherits the previous holder's tenant

This is the one that matters. GitHub frees a login when its owner renames or deletes
the account, and anyone may then claim it.

$ python3 -c "
from shared import tenancy
a = tenancy.userkey('github','alice')   # person 1
b = tenancy.userkey('github','alice')   # person 2, who later claimed the freed login
print(a); print(b); print('SAME TENANT:', a==b)"
gh-alice-ed66acf0
gh-alice-ed66acf0
SAME TENANT: True

So person 2 authenticates as alice, derives gh-alice-ed66acf0, and is routed to
person 1's store, topics and ntfy topic — reading their prompts, their agents' replies and
their notification stream. registry.parse's collision check does not fire, because from
its point of view there is one identity github/alice, listed once.

This defeats the property tenancy.userkey's own docstring claims:

The digest is the security control, not decoration. [...] two distinct canonical
identities cannot produce one key no matter how the slug collapses them.

The digest does hold up — it is collision-free over its input. The defect is that the
input does not distinguish two distinct humans, so "canonical identity" is doing less work
than the sentence implies. DESIGN_PHASE3.md §2.2 calls a GitHub login "already almost a
valid key" and considers only case-sensitivity; mutability is not discussed anywhere in
the phase documents.

Why the login is the wrong identifier, per GitHub

GitHub's own REST documentation says so, under GET /user/{account_id} — that endpoint

takes their durable user ID instead of their login, which can change over time.

And id is already in the response fetch_login parses — no extra API call, no extra
scope:

$ gh api user -q '{login,id,node_id}'
id=6954688  login=mrsabath  node_id=MDQ6VXNlcjY5NTQ2ODg=

Suggested fix

Derive from the id; keep the login for readability and audit only.

  1. fetch_login returns (login, id, error) — or a small frozen GhUser. The name
    fetch_login becomes a misnomer worth renaming.
  2. userkey("github", str(id)) becomes the key; the login feeds _slug so the key stays
    readable (gh-mrsabath-<digest over 6954688>). That keeps §2.3's "readable in
    kubectl get kafkatopics" property while making the hashed input durable. It does mean
    the slug can drift from the digest after a rename, which is cosmetic — but it should be
    stated in the docstring rather than discovered.
  3. ce_submitter should keep carrying the login: it is the human-meaningful audit
    label, and DESIGN_PHASE2.md §2.6 is about what it is worth, not about tenancy.
    Consider carrying the id as a separate attribute if attribution needs to survive a
    rename.
  4. ghauth.is_allowed still compares logins, which is right for an operator-written
    allow-list — but then the allow-list and the tenancy key no longer share an input, and
    _canonicalise's docstring explicitly requires they agree on case. That invariant
    needs restating rather than deleting: they are now answering different questions (may
    this person submit?
    vs which tenant is this?), and the test pinning their agreement
    should be rewritten to say so.

Migration

Not free, and this issue should not pretend otherwise. Any deployment with existing
per-user topics has keys derived from logins. Two cases:

  • Pre-GA, no real tenants. Change the derivation and accept that multi mode is not
    yet deployed anywhere. EB_TENANCY_MODE still defaults to single and §9's steps 5+
    are unimplemented, so this is likely the true state — which makes now the cheap
    moment to fix it.
  • With live tenants. The registry's recorded userkey is the migration seam: record
    the id in each entry, keep the recorded key authoritative (resolve_caller already
    prefers it — "the registry's recorded key is authoritative when present"), and derive
    from the id only for new signups.

Scope note

This is a Phase 3 defect, not a Phase 4 one. Filed separately from #897 (the Phase 4
design) rather than folded into it, per DESIGN_PHASE2.md §8.7: documenting a bug
honestly is not the same as accepting it, and a finding belongs with the code it
describes. #897's enrollment flow derives the same key, so it inherits this.

Verification

Both snippets above were run against eventing/ at dc1d432 with
shared/tenancy.py, eventbridge/ghauth.py and eventbridge/auth.py unmodified, and the
gh api user output is live. No cluster was involved — these are pure functions, which is
DESIGN_PHASE2.md §8.6's "cheap version" of running it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions