feat(eventing): GitHub sign-in and an approved-user list

[mrsabath]

Replaces the made-up identity from EB_AUTH_TOKENS with a real, externally verified one, and separates "I do not know you" from "I know you and you are not approved."

Implements rossoctl/rossoctl#2606, under the Event Identity demo epic rossoctl/rossoctl#2506. Builds on the eventing tree from #877.

The two rejections

Case	Response
No credential	401 + WWW-Authenticate
Unknown or revoked token	401
Real GitHub user, not on the approved list	403 — "mrsabath is not on the approved-user list"
Approved user	202, identity on the event

The 403 is the one worth demoing: a genuine, authenticated person, refused, and told which identity was refused so it is actionable. No WWW-Authenticate on a 403 — retrying with another credential is not the remedy.

Verified end to end against a real GitHub account

Not just unit tests — the full browser flow:

$ eventbridge-cli.py login
  Open https://github.com/login/device
  Enter code: CEF6-3786
✔ signed in as mrsabath
  token stored at ~/.config/rossoctl-eventing/token (mode 600)

Then, with that token:

• 202 on POST /v0/agents, agent ran, final=True, real reply
• 403 against an instance whose list is aslom,Alan-Cha
• ce_submitter:mrsabath + ce_submitter_iss:github on the Kafka requests topic
• A group of 5 — every member carried both attributes on the wire

The constraint that shapes the design

GitHub does not issue a verifiable token for user login. The device flow returns an opaque string — no signature, no claims, nothing to check offline. (The JWKS at token.actions.githubusercontent.com is for Actions workloads, not users.)

So EventBridge cannot verify locally; it must ask GitHub who holds the token via GET /user. Three consequences, all deliberate:

• Sign-in depends on GitHub being reachable. A failed lookup is a 401, never an allow — failing closed is the only safe direction for "who is this".
• The cache is load-bearing, not an optimisation. Without it every request spends one of 5000 hourly API calls and adds GitHub's latency to the request path. Keyed by sha256(token), so it never holds a usable credential. Failures are not cached, so a revoked token stops working promptly and an outage does not pin a legitimate user to a failure for the whole TTL.
• No scopes are requested. GET /user returns the login for an unscoped token, verified against the live API (x-accepted-oauth-scopes is empty). The app asks for the least access that answers the question — nothing here can read a repository.

Decisions worth reviewing

• An empty approved list denies everyone. The other reading — empty means everybody — would turn a missing environment variable into an open door.
• Logins compare case-insensitively, because GitHub logins are. Comparing exactly would refuse a genuinely approved user over capitalisation.
• EB_AUTH_TOKENS still works, as a fallback: it keeps tests off the network, keeps an offline demo possible, and gives an operator a break-glass credential when GitHub is unreachable.
• ce_submitter_iss records who vouched — github, or absent for a static token. Without it a reader cannot tell a verified identity from a name typed into an env var.
• The client id is a committed default. The device flow has no client secret, so unlike an ntfy topic it is not a capability. Override with EB_GITHUB_CLIENT_ID.
• login prints the code and blocks rather than opening a browser, which fails silently over SSH and in a container — where this is most often run.

Honest scope

ce_submitter is still unsigned, and Kafka is plaintext. The claim is "a real GitHub user, on an approved list, authorised this request" — not "the event proves it." Anything with write access to the topic can forge the attribute.

Making it provable needs submitter inside signing.SIGNED_ATTRS and a producer that signs. rossoctl/rossoctl#2607 covers that, and the kid plus approved-key-set groundwork for it is in aslom#3.

Also unchanged: /continue stays unauthenticated so the ntfy phone action keeps working — authenticating it would put a long-lived token in every notification traversing a public server. Per-correlation HMAC keys are the planned fix.

Tests

490 passed, 5 skipped — upstream main is 450/5, so this adds 40 and regresses nothing. No test touches the network; the device flow and GET /user are exercised through injected fakes.

ruff clean under the pinned 0.11.4.

Docs

The OAuth App setup is documented in rossoctl/rossoctl#2609 (docs/eventing/github-oauth-app.md), which also covers the scope reasoning and revocation.

Assisted-By: Claude Code

[aslom]

Reviewed at 2c94c9c, verified against a local checkout: 490 passed / 7 skipped on 3.14, ruff check and ruff format --check both clean, and all 11 CI checks green.

This is the Phase 2 work that #877 explicitly deferred, and it lands the hard part well. Three things I went looking for and found handled:

• The device flow follows GitHub's pacing contract — authorization_pending continues, slow_down adds five seconds and re-reads the interval, expired_token and access_denied are distinguished. sleep/now injected so the tests exercise pacing without waiting.
• No scopes are requested. GET /user answers the only question being asked, so a leaked token from this app cannot read a repository. That is the right least-privilege call and it's worth how prominently the docstring says so.
• The CLI's token write is genuinely careful. os.open(..., O_CREAT, 0o600) rather than write_text plus a later chmod, with the follow-up chmod kept precisely because os.open's mode applies only on creation. I tried to find a window here and could not reach one: the directory is created 0o700, so the pre-existing-loose-file case needs write access to a directory nobody else has.

401 vs 403 as distinct answers (§2.4) is the right decision and the reason given for it is the correct one.

No blocking issues. Three non-blocking findings, all documentation-or-efficiency rather than correctness:

1. The revocation claim does not hold (three places)

The claim that "failures are not cached [so] a revoked token must stop working promptly" appears in ghauth.py, test_ghauth.py and DESIGN_PHASE2.md §2.3. Not caching failures does not affect revocation at all — revocation is bounded by the positive TTL, because a cache hit short-circuits resolve() before fetch_login is ever called. Measured on this revision with an injected clock:

login  -> ('alice', None)
revoked on GitHub; token still presented:
  t+   0s  -> login='alice'   STILL AUTHENTICATES
  t+ 299s  -> login='alice'   STILL AUTHENTICATES
  t+ 300s  -> login=None      refused

So the window is the full 300 s default. The second half of the stated reasoning — that an outage must not pin a legitimate user to a failure for the whole TTL — is correct and is what not-caching-failures actually buys. The two reasons don't "pull the same way"; only one of them is load-bearing for this design.

Nothing here is insecure: the window is bounded, configurable via EB_GITHUB_CACHE_TTL_S, and 300 s is defensible. It's the stated property that needs correcting, and this codebase has been careful about exactly that — auth.py saying outright that submitter is unsigned is the standard I'm holding it to.

2. Break-glass pays a doomed GitHub round-trip on every request

Static tokens are checked only after GitHub resolution fails, and failures aren't cached, so a static-token request makes one GitHub call that can never succeed — every time. During the outage break-glass exists for, that's the full fetch_login timeout (5 s default) added to every request, against the same 5000/hour budget §2.3 says the cache exists to protect.

Checking the static map first would remove both costs: it's local, constant-time, and can't be shadowed in practice. The comment at auth.py:160 explains that static is tried, but not why GitHub goes first — if there's a reason, it's worth a line, because the current order makes the fallback slowest exactly when it's needed most.

3. auth.resolve's docstring says "and" where the code means "or"

It documents GitHub sign-in as active "when a client id and an approved-user list are configured", but github_on is an or. Your own test_github_on_with_only_an_allowed_list_still_enforces shows the or is deliberate, so this is the docstring being imprecise, not the code being wrong. Worth fixing because the two half-configured states behave quite differently — client-id-only refuses everyone with 403, list-only accepts any GitHub PAT from an approved login with no OAuth app involved.

---

Areas reviewed: device flow, token→login resolution, the cache, the approved-user list, 401/403 semantics, handler wiring on both create routes, CE attribute naming, CLI token storage, config precedence, tests, design doc
Author: mrsabath (MEMBER — maintainer)
Agent/IDE config (.claude/.vscode): none — gate clean, no matches in the diff
Commits: 3, all signed off (DCO green)
Verified locally at 2c94c9c: 490 passed / 7 skipped on 3.14; ruff check . and ruff format --check . clean
CI: 11/11 green

Approving — none of the above blocks.

One last note, because it's the part of 2c94c9c most worth keeping: the response to the submitter_iss slip was a generic guard rather than a rename. test_every_extension_attribute_name_is_cloudevents_compliant enumerates every EXT_* constant and regex-checks it, so the next non-compliant name is caught too — which matters precisely because, as the docstring says, the codec validates nothing and a bad name round-trips locally before being dropped by a spec-compliant consumer a hop later. I checked it holds: it covers all 12 extension attributes today, and re-introducing submitter_iss makes it fail. The 20-char SHOULD-limit test alongside it is the same instinct.

[aslom]

suggestion — not caching failures does nothing for revocation; the positive TTL is what governs it.

resolve() returns on a cache hit before fetch_login is ever called, so once a token has resolved successfully, revoking it on GitHub has no effect until the entry expires. Measured on this revision with an injected clock:

login  -> ('alice', None)
revoked on GitHub; token still presented:
  t+   0s  -> login='alice'   STILL AUTHENTICATES
  t+  60s  -> login='alice'   STILL AUTHENTICATES
  t+ 299s  -> login='alice'   STILL AUTHENTICATES
  t+ 300s  -> login=None      refused

The second reason is correct and is the real one — not caching failures is what stops a GitHub outage pinning a legitimate user to a failure for the whole TTL. The two just don't pull the same way, and the revocation half overstates what the design delivers.

Nothing here is insecure: the window is bounded, EB_GITHUB_CACHE_TTL_S tunes it, and 300 s is a fine default. It's worth stating accurately because the honesty of the docstrings is doing real work elsewhere in this package — auth.py saying plainly that submitter is unsigned is the standard I'm holding this to. The same sentence is in DESIGN_PHASE2.md §2.3 and in test_resolve_does_not_cache_a_failure's docstring.

Suggested change

	Negative results are not cached. A revoked token must stop working promptly,
	and a GitHub outage must not pin a legitimate user to a failure for the whole
	TTL.
	Negative results are not cached, so a GitHub outage does not pin a legitimate
	user to a failure for the whole TTL. Note this does **not** make revocation
	prompt: a successful lookup is cached, and `resolve()` returns from the cache
	without calling GitHub, so a token revoked upstream keeps authenticating until
	its entry expires — up to `ttl_s` (300 s by default). Lower the TTL if that
	window matters more than the API budget.

[aslom]

suggestion — the break-glass path makes a GitHub call that can never succeed, on every request.

A static token is not a GitHub token, so ghauth.resolve above always fails for one — and since failures aren't cached, that doomed call is repeated per request rather than once. Two costs, both landing on the path that exists for when things are already going wrong:

• During a GitHub outage, every break-glass request waits the full fetch_login timeout (5 s default) before the static check runs. The fallback is slowest exactly when it is needed most.
• Each one spends a call from the 5000/hour budget that §2.3 gives as the reason the cache is load-bearing.

Checking the static map first avoids both. It's a local hmac.compare_digest against a handful of entries, so it costs nothing, and it can't be shadowed in practice — a GitHub token would have to be byte-identical to a configured static token.

The comment explains that static is tried before giving up, which is the useful half. If there's a deliberate reason GitHub goes first — precedence, or not wanting a stale static entry to mask a real identity — that's worth the extra line, because the ordering is not self-evidently the cheap way round.

[aslom]

nit — says "and", means "or".

github_on at line 144 is bool(github_client_id) or bool(allowed_users), and test_github_on_with_only_an_allowed_list_still_enforces shows the or is intended — so the code is right and this sentence is the imprecise part.

Worth correcting rather than leaving, because the two half-configured states are quite different and an operator reading this would predict neither: client-id-only sends every caller down the GitHub path and then refuses all of them with 403 (an empty allowed_users denies, per is_allowed), while list-only accepts any valid GitHub PAT belonging to an approved login, with no OAuth App involved at all. Both fail closed, which is the right instinct — they're just not what "and" describes.

Suggested change

	1. **GitHub sign-in**, when a client id and an approved-user list are
	configured. The real path.
	1. **GitHub sign-in**, when *either* a client id or an approved-user list is
	configured. The real path. Note the consequences of configuring only one:
	a client id alone refuses everyone (an empty approved list denies), and an
	approved list alone accepts any GitHub token belonging to an approved
	login without an OAuth App being involved.

[mrsabath]

Thanks — all three addressed in 304925d. The first one was a genuine error in a claim I had made prominently, which is the worst kind to leave standing.

1. The revocation claim. You're right, and I reproduced your measurement before changing anything — accepted at t+299 s, refused at t+300 s. Not caching failures does nothing for revocation, because a cache hit short-circuits resolve() before fetch_login ever runs. I had conflated two unrelated properties and asserted they "pull the same way"; only the outage one is load-bearing.

Corrected in all three places, and the doc now says outright that an earlier revision claimed otherwise rather than quietly editing it. Added test_a_revoked_token_keeps_working_until_its_positive_entry_expires and a shorter-TTL case, so the window is pinned by a test instead of by prose. You held it to the standard auth.py set about submitter being unsigned — that's the right comparison, and the fix is a test, not better wording.

2. Break-glass ordering. Reordered: static tokens are now checked first. Your framing was the convincing part — the fallback was slowest exactly when it was needed, and it spent the budget §2.3 says the cache exists to protect. There was no reason for GitHub-first; I simply wrote the branches in the order I thought about them. Two tests now cover it: one asserting no GitHub call happens at all for a static token, one asserting real sign-in still resolves so the reorder shadows nothing.

3. The docstring and/or. Fixed, and I took your point further — the docstring now spells out what each half-configured state does, since they diverge sharply: client-id-only 403s everyone, list-only accepts any PAT from an approved login with no OAuth App involved. Worth knowing which mistake you've made.

496 passed, 5 skipped (was 492), ruff check and ruff format --check both clean.

On LoginCache growth, which you didn't flag but is adjacent to #1: still deliberately unbounded, now with the reasoning in the commit rather than assumed. Bounded by the approved-user count, failures uncached. If this outlives the demo it wants an LRU bound — worth an issue at that point rather than speculative code now.

Next is #2607, where the interesting half of that review will be the kid allowlist and the two kafka_in hazards: the decode/store path is outside a try, so verification has to degrade rather than raise, and group events are self-published with no correlationid.