Repository navigation
feat(control-plane): test-user logins behind a shared secret - #620
Conversation
2c52de5 to
d4ec0a4
Compare
ingpaolodettori-dev
left a comment
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Re-review at d4ec0a4 (force-push of the single commit, same base d8bc58b).
Fixed since 2c52de5: SH_TEST_USERS ref is now optional (closes the SBX0 missing-key issue); base-control-plane.test.ts updated for the fourth secret item.
Still blocking:
- With test users off,
moca-mu1holds an emptySH_TEST_USER_SECRETand the volume projects it as a 0-byte file.SH_TEST_USER_SECRETis inCONTROL_PLANE_SECRETS, andcredentialValuethrowssystemd credential … is emptyon it (no ENOENT fallback), so the control plane crash-loops on every default deploy. The new commit treats the k8s-kind-e2e failure as a missing key, but on a fresh stack the key was already present (empty); the fix makes the empty file more common rather than less. Suggest a projected volume with a separate,optionalmoca-test-usersSecret written only when SH_TEST_USERS>0. - Turning test users off after they were on still keeps the stored secret (setup.sh:760), so boot fails with
SH_TEST_USER_SECRET is set but SH_TEST_USERS is not.
The branch also still conflicts with main (mocactl/src/cli.ts, headless.ts, from main's --no-browser/LoginHelpers change), so no CI beyond DCO has run on this head. Earlier suggestions/nits on unchanged files are repeated inline.
Author: aslom (MEMBER — maintainer)
Agent/IDE config (.claude/.vscode): none
Commits: 1, signed-off: yes
CI status: not run (mergeable_state=dirty)
| # leave this pod unable to mount and stuck in ContainerCreating. setup.sh therefore | ||
| # patches the key in on every run (empty when test users are off, which is a valid | ||
| # stored state), and identity.ts refuses an empty one alongside SH_TEST_USERS. | ||
| - { key: SH_TEST_USER_SECRET, path: SH_TEST_USER_SECRET } |
There was a problem hiding this comment.
must-fix: Still crash-loops a default deploy, even with the key present. The key does exist on a fresh stack: setup.sh wrote it at the previous head too. The problem is its empty value. Kubelet projects it as a 0-byte file, SH_TEST_USER_SECRET is in CONTROL_PLANE_SECRETS (main.ts:120), and credentialValue throws systemd credential … is empty (systemd-credentials.ts:33) before identity.ts runs. Unlike ENOENT, it doesn't fall back. I reproduced this against this head's systemd-credentials.ts. So "an empty value is a legitimate stored state" (here and setup.sh:755) is not true for the pod. optional is whole-source on a secret volume, but a projected volume can mix a required moca-mu1 source with a separate moca-test-users Secret marked optional: true. Write that Secret only when SH_TEST_USERS>0 and delete it when off. Then a missing file is ENOENT → unset, and neither old nor new stacks need an empty key. Alternatively, let credentialValue treat an empty file as unset for this one name, but that weakens the general "empty is refused" rule.
| # So the early return also requires the test-user key to be present AND already what we want. | ||
| if [[ -n "$priv" && -n "$kek" && -n "$xchg" && -n "$tsecret_key_present" && | ||
| "$tsecret" == "$stored_tsecret" ]]; then | ||
| if ((TEST_USERS == 0)) || [[ -n "$tsecret" ]]; then return 0; fi |
There was a problem hiding this comment.
must-fix: Still open from the previous head. When test users go off after being on (SH_TEST_USERS=), tsecret == stored_tsecret (non-empty), the key is present and TEST_USERS == 0, so this returns early and keeps the secret. On the next boot testUserSecretFromEnv throws SH_TEST_USER_SECRET is set but SH_TEST_USERS is not. When TEST_USERS==0, clear or remove the secret (with the separate-Secret design from #1, delete moca-test-users), and add a setup.test.sh case for on→off that asserts it.
| ! grep -q 'run moca-genkeys' "$MOCK_LOG" || | ||
| fail 'patching the test-user key in started the key generator, though every other value existed' | ||
| # With test users off the value is empty, which is a legitimate stored state -- the KEY must exist. | ||
| [[ -z "$(sv moca moca-mu1 SH_TEST_USER_SECRET)" ]] || fail 'test users off must leave the value empty' |
There was a problem hiding this comment.
suggestion: This asserts the exact state that breaks boot: an empty SH_TEST_USER_SECRET in moca-mu1 with the volume projecting it. The setup test passes because the mock never starts a pod. Consider a control-plane unit test that runs withCredentials(env, CONTROL_PLANE_SECRETS) over a CREDENTIALS_DIRECTORY shaped like the k8s volume with test users off. That would have caught this without kind.
| { | ||
| method: 'POST', | ||
| path: '/v1/auth/test-token', | ||
| auth: 'none', |
There was a problem hiding this comment.
suggestion: Carried from 2c52de5. The route is mounted even when the feature is off, so server.ts validates the body before the handler's 404. An empty body gets 400 invalid_json, and malformed JSON gets 400, while a truly unrouted path gets 404. That contradicts the "indistinguishable from an unrouted path" claim in README/ADR/openapi. Either skip the route in matchRoute when deps.testUsers is absent, or parse after the provider check. Add a server-level empty-body test.
| if (typeof secret !== 'string' || !provider.authenticates(secret)) { | ||
| // One code for a missing and a wrong secret alike: distinguishing them tells a prober which | ||
| // half of the problem to work on. | ||
| throw new CpError('unauthorized', 'test-user login refused: wrong or missing secret'); |
There was a problem hiding this comment.
suggestion: Carried. A refused secret leaves no audit, log or rate limit on an endpoint that mints real logins (possibly an admin, if test:N is in SH_ADMIN_SUBJECTS). An operator-supplied secret needs only 16 chars. At least log or audit the refused attempts, without the presented value.
| * SBX0 §4.1: the sandbox profile needs no credential store, KEK, exchange token or inference | ||
| * settings, so none is required; the harness-only fields take inert values that no mounted route reads. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
nit: Carried. testUserProvider splits sandboxProfileConfig's doc comment from its function. Move it above.
| /** 'pending' until approved; 'expired' once the code lapsed unapproved (start a new one). */ | ||
| pollDeviceAuth(deviceCode: string): Promise<ApiLogin | 'pending' | 'expired'>; | ||
| /** The next API token and refresh token. ApiError `invalid_grant` means "log in again". */ | ||
| /** ADR-0045: a test-user login, gated by the control plane's shared secret. */ |
There was a problem hiding this comment.
nit: Carried. The new member sits between refreshAuth's doc comment and refreshAuth. Move it above.
| 'control-plane-url': { type: 'string' }, | ||
| 'harness-url': { type: 'string' }, | ||
| session: { type: 'string' }, | ||
| 'test-user': { type: 'string' }, |
There was a problem hiding this comment.
nit: Carried. --test-user is silently ignored on commands other than login. Add a stray-flag guard like --all/--config/--repo have.
| # Each user's list holds only their own sessions. | ||
| ok_list, ids1 = driver.list_sessions(u1) | ||
| cross.append(Claim("user 1's session list excludes user 2's", | ||
| ok_list and u2.session_id not in ids1, |
There was a problem hiding this comment.
suggestion: Carried. Passes vacuously when user 2 has no session ('' not in ids1). Require u2.session_id. Likewise, an empty u1.session_id skips the ownership claims at 340-352 instead of failing them.
| }, indent=2)) | ||
| return 0 if passed else 1 | ||
| finally: | ||
| if args.keep or args.api: |
There was a problem hiding this comment.
nit: Carried. In --api mode, rundir from mkdtemp (line 322) is never removed.
d4ec0a4 to
a6f2f73
Compare
ingpaolodettori-dev
left a comment
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Follow-up at a6f2f73. This is a rebase onto c3bfa26, and the cli.ts/headless.ts conflict resolution with main's --no-browser is correct. Every other file is identical to d4ec0a4, so the earlier items are still open, including both must-fixes: (1) control-plane.yaml:140 projects an empty SH_TEST_USER_SECRET when test users are off, credentialValue throws on an empty credential file, and the control plane crash-loops; (2) setup.sh:760 keeps the secret on an on→off switch, so testUserSecretFromEnv throws on boot. For nit #11, the new --no-browser stray-flag guard is the pattern to copy for --test-user.
a6f2f73 to
753098a
Compare
ingpaolodettori-dev
left a comment
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Thanks for the rework: all 13 items from the previous rounds are verified fixed at this head. That includes both must-fixes: the test-user secret now lives in its own moca-test-users Secret, from an optional projected source, deleted when off, and covered by the on→off test. There is also the unmounted route with server-level 404 tests, the refused-login audit, the --target ocp ack, 10# normalisation, the stray-flag guard, and the test-users.py prerequisites.
One new must-fix: deploy/k8s/overlays/ocp-sbx/patch-control-plane.yaml still patches the credentials volume with secret:. The strategic merge (volumes are keyed by name) keeps the base's new projected: alongside it. kubectl kustomize deploy/k8s/overlays/ocp-sbx at this head renders the control-plane credentials volume with both projected (moca-mu1 KEK + exchange token, moca-test-users) and secret (signing key + janitor token). The API server refuses a volume with more than one type, so SBX0 deploys break. Even if it were accepted, the SBX control plane would mount the KEK/exchange token that §4.1 removed. deploy/sbx/tests/overlay.test.sh:121 checks for exactly that, so deploy-scripts should go red once CI runs. Adding projected: null to the overlay's credentials volume renders a plain secret volume again (checked locally). Please also add an overlay test that the volume has exactly one source type.
Also:
- nit: the test-user Secret isn't part of
moca.dev/settings-hash. ChangingSH_TEST_USER_SECRETwith the same count rewrites the Secret, but the running control plane keeps the old value until it restarts. Hash a digest of it, or document the restart. - The branch conflicts with main (
deploy/k8s/README.mdSecret table, where main addedprovisionerto moca-redis;docs/adrs/README.md, where ADR-0046 landed), so only DCO has run on this head. Please rebase so the full CI runs.
| - { key: SH_SESSION_TOKEN_PRIVATE_KEY, path: SH_SESSION_TOKEN_PRIVATE_KEY } | ||
| - { key: SH_CREDENTIAL_KEK, path: SH_CREDENTIAL_KEK } | ||
| - { key: SH_EXCHANGE_TOKEN, path: SH_EXCHANGE_TOKEN } | ||
| projected: |
There was a problem hiding this comment.
must-fix: overlays/ocp-sbx/patch-control-plane.yaml still patches this volume with secret:, and a strategic merge keeps this projected: alongside it. At this head kubectl kustomize deploy/k8s/overlays/ocp-sbx renders credentials with both projected and secret. The API server refuses that (more than one volume type), and if it didn't, the SBX control plane would mount SH_CREDENTIAL_KEK/SH_EXCHANGE_TOKEN (deploy/sbx/tests/overlay.test.sh:121 fails on exactly that). Add projected: null to the overlay's credentials volume (renders a plain secret volume again, checked locally), plus an overlay.test.sh assertion that it has exactly one source type.
| maxBodyBytes?: number; | ||
| /** Stable key the router and the OpenAPI drift test both use to name the operation. */ | ||
| operationId: string; | ||
| operationId: string; /** |
There was a problem hiding this comment.
nit: the new feature JSDoc opens on the same line as operationId: string;. Put /** on its own line, or Prettier (lint) will likely reformat it.
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Responses to review on #620
Pushed as 753098ae (rebased onto c3bfa26c, still one commit). Every point below is addressed;
two needed a design change rather than a patch.
must-fix 1 — control-plane.yaml:140, empty secret still crash-loops
You were right, and my previous comment was wrong. I reproduced your finding before changing
anything: a 0-byte SH_TEST_USER_SECRET in a directory shaped like the mounted volume gives
SH_TEST_USER_SECRET: systemd credential /…/SH_TEST_USER_SECRET is empty
credentialValue returns fromEnv only for ENOENT and throws on an empty file
(systemd-credentials.ts:33), so this fires inside withCredentials before identity.ts is ever
reached. My claim that "an empty value is a legitimate stored state" was false for the pod, and it
is exactly the k8s-kind-e2e failure. Thank you for pinning it that precisely.
Taken your first option. credentials is now a projected volume with two sources: a required
moca-mu1 and a separate moca-test-users marked optional: true. setup.sh writes that Secret
only when SH_TEST_USERS > 0 and deletes it when they go off, so with test users off the file
is absent → ENOENT → unset, which the existing code already handles. No empty key anywhere, and
old stacks boot untouched.
I did not take the alternative (teaching credentialValue that one name may be empty): it weakens
a general rule for one caller, and the invariant "absent is the only 'off'" is easier to keep right.
must-fix 2 — setup.sh:760, on→off leaves the secret behind
Also correct, and fixed by the same change. The test-user secret is no longer entangled with
ensure_mu1_secret's early return; it has its own function whose first branch is:
if ((TEST_USERS == 0)); then
# Deleted, not emptied: absent is the only state the credential loader reads as unset.
kc delete secret moca-test-users -n "$NS" --ignore-not-found >/dev/null
return 0
fiWith the setup.test.sh case you asked for: off → on → re-run → SH_TEST_USERS= asserts the
Secret appears with a 64-hex value, is not rotated by a plain re-run, is absent again after
off, and never appears in moca-mu1. The mock gained a delete secret handler, otherwise that
last assertion would have passed without the delete actually happening.
suggestion — setup.test.sh:473, a unit test that would have caught it without kind
Added, in packages/control-plane/test/main.test.ts: three cases running
withCredentials(env, CONTROL_PLANE_SECRETS) over a directory shaped like the mounted volume —
absent (boots, reads as 0 users), present but empty (throws, the regression itself), and present
with a value (reads it, and testUserSecretFromEnv accepts it). That is the boundary the setup test
structurally cannot see, as you noted.
suggestion — routes.ts:101, the 404 claim vs. body validation
Confirmed: server.ts parses and validates the body before dispatch, so a mounted route answers
400 invalid_json to an empty body and thereby reveals itself. Took your first option — the route
is now unmounted. RouteSpec gained an optional feature field, matchRoute takes a
RouteFeatures argument, and server.ts passes { testUsers: !!deps.testUsers }.
With the server-level test you asked for: with the feature off, /v1/auth/test-token and a
genuinely unrouted path both answer 404 not_found for an empty body, malformed JSON and a valid
body alike. With it on, the empty body becomes a 400 — which is now the honest signal that the route
exists. The handler's if (!provider) throw 404 stays as belt-and-braces for a future wiring
mistake, and says so.
suggestion — handlers.ts:503, no audit on a refused secret
Added. A refusal now emits decision: test_user_login_refused with subject: '-' and
reason: wrong_secret | no_secret, following refresh_refused's anonymous-audit shape. The
presented value is never recorded. No rate limit yet — that belongs with the control plane's
general request limiting rather than one endpoint, and I did not want to invent a scheme here; say
the word if you'd rather it block before merge.
suggestion — setup.sh:494, sticky on --target ocp
Agreed. --target ocp now refuses test users unless
SH_TEST_USERS_ACK=i-know-this-is-not-for-production is set, naming SH_TEST_USERS= as the other
way out. kind and ocp-single are dev/test targets by definition and need no acknowledgement. Tested
both directions, and documented in §13 and the configuration reference.
nit — setup.sh:493, leading zeros
Fixed with your suggestion: TEST_USERS="$((10#$TEST_USERS))" right after the regex, so 08 no
longer dies with a misleading "at most 1000" and 010 is stored as 10.
nit — setup.sh:1044, stray blank line and the "nine keys" header
Both fixed: blank line removed, header now says ten keys.
nit — main.ts:182, split doc comment
Fixed — testUserProvider moved above, so the SBX0 comment sits with sandboxProfileConfig again.
nit — types.ts:125, same problem
Fixed — testUserToken and its own comment now sit above refreshAuth's.
nit — cli.ts:133, --test-user silently ignored elsewhere
Fixed — a stray-flag guard matching --no-browser's, immediately after it.
suggestion — test-users.py:360, vacuous cross-user claims
Correct on both counts. Both session ids are now a prerequisite claim that fails loudly when
either is missing, instead of an if that skipped the ownership checks; the exclusion also asserts
u1.session_id in ids1 so it cannot be satisfied by an empty id.
nit — test-users.py:402, --api leaks its tempdir
Fixed — --keep is now the only thing that preserves rundir, in both modes.
Verification
pnpm -r typecheck |
clean |
packages/control-plane |
858 pass (900 with Redis up; +9 new) |
supervisor k8s manifest tests |
104 pass |
deploy/k8s/tests/setup.test.sh |
105 pass (+2 new) |
lint (pre-commit --all-files) |
all hooks pass |
check-links |
209 files, 0 broken |
Remaining full-suite failures are pre-existing flakes in files this PR does not touch —
handoff.integration.test.ts (fails 2-of-3 in isolation on its own), the mocactl ink overlays, and
an sbx subdirectory test from #611. Happy to file those separately if useful.
Still owed
The live N=10 re-run on OpenShift against this build. The previous head passed 10/10 plus 4/4
cross-user claims, but that was before the projected-volume change, so it needs redoing — and the
projected volume is precisely what the e2e exercises, so CI on this push is the first real check.
753098a to
8937c2f
Compare
ingpaolodettori-dev
left a comment
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Follow-up on 8937c2f. The code changes look good: the ocp-sbx projected: null fix and the settings-hash fix both check out (verified with kubectl kustomize), and we would approve otherwise. Two things need fixing before this can go in:
- The required
checkjob is red on this head. ItsTeststep (pnpm -r test) exits 1 after the typecheck passes (run 40533836790).checkis green on main, so please find the failing test and fix it, or re-run the job if you're confident it's one of the known flakes (handoff.integration, mocactl ink overlays). If it passes on a re-run, please mention which test flaked. - The branch conflicts with main again (
mergeable: CONFLICTING; main has moved 12 commits past the base). Please rebase. That also triggers a fresh CI run.
Once check is green on the rebased head, we'll approve.
8937c2f to
53d8764
Compare
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Rebased onto 916014df as 53d8764, still one commit. Both blockers addressed.
1. The red check job — the handoff.integration flake
It is the known one, and I measured it rather than assuming. The failing test is:
packages/supervisor/test/handoff.integration.test.ts
× socket hand-off, end to end > spreads connections across workers by least-in-flight
Running just test/main.test.ts test/handoff.integration.test.ts three times each, same machine,
back to back:
| runs failed | |
|---|---|
clean main (2e53d445) |
2 of 3 |
| this branch | 1 of 3 |
Same test, same assertion (expected 1 to be 2), and neither that file nor
packages/supervisor/src is in this branch's diff. main fails it more often than the branch
does, so this is the pre-existing flake, not a regression.
One thing worth knowing about how it presents: pnpm -r stops at the first failing package, so
a supervisor flake masks every package after it — including control-plane, where all of this
PR's code lives. Run directly, control-plane is 1010/1010 on this head.
On the rebased head I also saw two harness getBundleRedis reconnect tests fail inside the full
pnpm -r test, which pass 763/763 twice when harness is run on its own — load-induced, same
shape. Neither is in the diff either.
So: a re-run is the right call, and if CI stays red on something other than those, that would be
a real signal worth chasing.
2. The conflict — rebased, and the two gates are now one mechanism
The conflict was in routes.ts and server.ts, and it was a genuine one rather than adjacent
lines: main had independently added the same idea for a different feature —
RouteSpec.requiresEgress plus matchRoute(..., opts: { egress?: boolean }) — while this branch
added RouteSpec.feature plus matchRoute(..., features: RouteFeatures).
Rather than carry two parallel option objects, I folded mine into theirs. RouteFeatures now holds
both gates and matchRoute takes one opts:
export interface RouteFeatures {
/** `config.sandbox.egress` is configured: mounts the credential routes on the sandbox profile. */
egress?: boolean;
/** SH_TEST_USERS > 0: mounts POST /v1/auth/test-token. */
testUsers?: boolean;
}
// ...
if (profile === 'sandbox' && route.requiresEgress && !opts.egress) continue;
if (route.feature && !opts[route.feature]) continue;The two conditions stay separate because they are not the same rule — requiresEgress is
per-profile, feature is global — but they share one options bag, so the next gate has an obvious
home. server.ts passes both. feature is typed 'testUsers' rather than
keyof RouteFeatures, so adding egress to the interface cannot accidentally make it a valid
feature value.
The merge had also welded this branch's doc comment onto operationId's line; fixed.
Verification on 53d8764
pnpm -r typecheck |
clean |
packages/control-plane |
1010 pass (Redis up) |
harness |
763 pass |
supervisor k8s manifest tests |
105 pass |
deploy/k8s/tests/setup.test.sh |
107 pass |
deploy/sbx/tests/overlay.test.sh |
16 pass |
check-links |
0 broken |
lint (pre-commit --all-files) |
all hooks pass |
Thanks for verifying the projected: null and settings-hash fixes with kubectl kustomize — and
for the earlier catch on the overlay. That one was a real break I had missed, and the
one-source-type assertion it prompted is now in overlay.test.sh.
Still owed, and I would rather say so than let it pass quietly
The live N=10 run on OpenShift has not been redone since the projected-volume rework. The
earlier run passed 10/10 users and 4/4 cross-user claims, but that was two designs ago; the
projected volume is exactly what k8s-kind-e2e exercises, so CI is the current evidence and a
fresh manual run is still worth doing before anyone relies on this on a real cluster.
ingpaolodettori-dev
left a comment
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Follow-up on 8937c2f. The rebase looks correct. The only change from 8937c2f is the routes.ts/server.ts conflict with #630, and folding testUsers into RouteFeatures alongside egress keeps main's per-profile requiresEgress check intact. feature?: 'testUsers' is a good guard. The JSDoc placement nit is fixed too. Re-rendered kubectl kustomize deploy/k8s/overlays/ocp-sbx at this head, and credentials is still a single secret volume.
On check: thanks for measuring the handoff.integration flake. It's consistent with main's own check/Test failure at 0b2339b today. Approving on a green re-run. The earlier nits on --test-user stray-flag handling and the test-users.py vacuous pass stay open as non-blocking follow-ups.
Fixes rossoctl#616 Every login to the control plane is GitHub's device flow, which needs a person with a browser. smoke.sh works around that by minting an API token inside the control-plane pod with the signing key, which skips refresh.issue(), rolesFor() and the audit login entry -- so the smoke authenticates in a shape no real user has, and needs kubectl exec to do it. Multi-user behaviour (per-subject credentials, cross-user session denial) had no automated coverage above one user. With SH_TEST_USERS=N the control plane now serves logins for N synthetic subjects test:1..test:N via POST /v1/auth/test-token, and `mocactl login --test-user N` drives it. A test user is configured exactly like a GitHub user: the handler makes the same two calls as completeDeviceAuth -- refresh.issue() then loginBody() -- so token TTLs, rotation, roles, ownership, audit and credential storage cannot tell them apart. Only the way identity is established differs. The path is gated by SH_TEST_USER_SECRET, which the client presents and the control plane compares in constant time (both sides hashed to 32 bytes first, so a length mismatch is neither an exception nor an oracle). The endpoint issues a real login for an arbitrary subject, so without a secret anyone who could route to the control plane would be every test user -- including a compromised sandbox. The secret is a credential FILE via CONTROL_PLANE_SECRETS, never an env line a kubectl child inherits, and setup.sh generates it into moca-mu1 without rotating it on a re-run. Fails closed: 404 when SH_TEST_USERS is unset (indistinguishable from an unrouted path), the route is not mounted on the sandbox profile at all and the control plane refuses to boot if the two are combined, N is capped at 1000, the secret must be at least 16 characters, and a warning names the count at boot and on every setup run. deploy/k8s/test-users.py drives N users (default 10) through real mocactl commands, one XDG_CONFIG_HOME each -- auth.json holds exactly one login, so one directory cannot hold ten -- and asserts the cross-user claims: user 2 is refused user 1's real session, user 1 still resumes it, and each list holds only its owner's sessions. --api runs the same claims over HTTP as a reference for a non-CLI client. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Aleksander Slominski <aslom@us.ibm.com>
53d8764 to
0c1ccd7
Compare
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Rebased onto 5bcff7a6 as 0c1ccd7, still one commit, no conflicts.
The check failure should be gone
It was not the handoff.integration flake I measured last round — or rather, that was only part of
it. The actual blocker was a second, deterministic timeout that I had misattributed to contention:
packages/mocactl/test/sbx-push.test.ts
× the push list (SBX0 §6.2) > splits a large tree into several Execs (timed out in 120000ms)
Measured per phase, that test spent 0.6 s in planPush, 1.3 s in pushTree, and 19.6 s in the
assertions — almost all of it on one line comparing two ~12 MiB buffers with toEqual, which
walks them element by element. Buffer.equals asserts the same thing in ~0 ms. It reproduced with
all 128 sibling tests skipped, so it was never about parallelism; I was wrong to call it starvation.
That is fixed on main now (#642, merged as 5bcff7a), so this rebase picks it up rather than
carrying a duplicate. On the rebased head the test runs in 2667 ms, down from 23967 ms, and
still fails if a single byte of the pushed copy differs — I checked by flipping one.
So check should now be red only if something real is wrong, which is the point.
What remains, and it is the flake you already accepted
pnpm -r test on this head still shows the supervisor pair:
× shutdown (§3.9) > stops after the grace period when a turn never finishes
× socket hand-off, end to end > spreads connections across workers by least-in-flight
Three consecutive runs of that package on this head gave 2, 1 and 0 failures — same code, same
machine. Neither file is in this branch's diff, and you confirmed main's own check fails the
same way at 0b2339b. pnpm -r stops at the first failing package, so this one also masks
everything after supervisor.
One honest correction to my own numbers: an earlier run of this verification reported 57 mocactl
failures, which alarmed me. Re-running on an otherwise-idle machine gave 7, all of them the same
Test timed out in 5000ms with no assertion failures, and the two worst files passed 16/16 in
isolation. The 57 was my own concurrent test runs competing for CPU — worth saying plainly so the
number is not read as a signal.
Verification on 0c1ccd7
pnpm -r typecheck |
clean |
packages/control-plane |
1010 pass |
packages/mocactl |
1551 pass, 7 timeout-flakes (0 assertion failures) |
supervisor k8s manifest tests |
105 pass |
sbx-push suite (the formerly-failing one) |
129 pass, the big test now 2667 ms |
deploy/k8s/tests/setup.test.sh |
107 pass |
deploy/sbx/tests/overlay.test.sh |
16 pass |
lint (pre-commit --all-files) |
7/7 hooks |
check-links |
212 files, 0 broken |
Also worth noting from the last CI run: k8s-kind-e2e passed on 53d8764. That was the job the
projected-volume rework was for, and it is the first green run of it on this branch.
The two follow-ups you left open
Both are already in this commit, so they should not carry forward:
--test-userstray-flag handling —cli.tshas the guard next to--no-browser's:
--test-user only applies to \mocactl login``, exit 2.test-users.pyvacuous pass — both session ids are now a prerequisite claim that fails loudly
when either is missing, and the exclusion assertsu1.session_id in ids1so it cannot be
satisfied by an empty id.
If you meant something narrower by either, say which and I will take another look.
Still owed
The live N=10 run on OpenShift has not been redone since the projected-volume rework. The earlier
run passed 10/10 users and 4/4 cross-user claims, but that was two designs ago; k8s-kind-e2e going
green is the current evidence, and a fresh manual run is still worth doing before anyone relies on
this on a real cluster.
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Rebased onto 5bcff7a6 as 0c1ccd7, still one commit, no conflicts.
The check failure should be gone
It was not the handoff.integration flake I measured last round — or rather, that was only part of
it. The actual blocker was a second, deterministic timeout that I had misattributed to contention:
packages/mocactl/test/sbx-push.test.ts
× the push list (SBX0 §6.2) > splits a large tree into several Execs (timed out in 120000ms)
Measured per phase, that test spent 0.6 s in planPush, 1.3 s in pushTree, and 19.6 s in the
assertions — almost all of it on one line comparing two ~12 MiB buffers with toEqual, which
walks them element by element. Buffer.equals asserts the same thing in ~0 ms. It reproduced with
all 128 sibling tests skipped, so it was never about parallelism; I was wrong to call it starvation.
That is fixed on main now (#642, merged as 5bcff7a), so this rebase picks it up rather than
carrying a duplicate. On the rebased head the test runs in 2667 ms, down from 23967 ms, and
still fails if a single byte of the pushed copy differs — I checked by flipping one.
So check should now be red only if something real is wrong, which is the point.
What remains, and it is the flake you already accepted
pnpm -r test on this head still shows the supervisor pair:
× shutdown (§3.9) > stops after the grace period when a turn never finishes
× socket hand-off, end to end > spreads connections across workers by least-in-flight
Three consecutive runs of that package on this head gave 2, 1 and 0 failures — same code, same
machine. Neither file is in this branch's diff, and you confirmed main's own check fails the
same way at 0b2339b. pnpm -r stops at the first failing package, so this one also masks
everything after supervisor.
One honest correction to my own numbers: an earlier run of this verification reported 57 mocactl
failures, which alarmed me. Re-running on an otherwise-idle machine gave 7, all of them the same
Test timed out in 5000ms with no assertion failures, and the two worst files passed 16/16 in
isolation. The 57 was my own concurrent test runs competing for CPU — worth saying plainly so the
number is not read as a signal.
Verification on 0c1ccd7
pnpm -r typecheck |
clean |
packages/control-plane |
1010 pass |
packages/mocactl |
1551 pass, 7 timeout-flakes (0 assertion failures) |
supervisor k8s manifest tests |
105 pass |
sbx-push suite (the formerly-failing one) |
129 pass, the big test now 2667 ms |
deploy/k8s/tests/setup.test.sh |
107 pass |
deploy/sbx/tests/overlay.test.sh |
16 pass |
lint (pre-commit --all-files) |
7/7 hooks |
check-links |
212 files, 0 broken |
Also worth noting from the last CI run: k8s-kind-e2e passed on 53d8764. That was the job the
projected-volume rework was for, and it is the first green run of it on this branch.
The two follow-ups you left open
Both are already in this commit, so they should not carry forward:
--test-userstray-flag handling —cli.tshas the guard next to--no-browser's:
--test-user only applies to \mocactl login``, exit 2.test-users.pyvacuous pass — both session ids are now a prerequisite claim that fails loudly
when either is missing, and the exclusion assertsu1.session_id in ids1so it cannot be
satisfied by an empty id.
If you meant something narrower by either, say which and I will take another look.
Still owed
The live N=10 run on OpenShift has not been redone since the projected-volume rework. The earlier
run passed 10/10 users and 4/4 cross-user claims, but that was two designs ago; k8s-kind-e2e going
green is the current evidence, and a fresh manual run is still worth doing before anyone relies on
this on a real cluster.
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
A correction to my previous comment, before it misleads anyone reading back.
I wrote that the only failure left on this head was the supervisor handoff.integration flake.
That was wrong. proto was also red, and not from a flake — go test -race ./... was reporting
a genuine data race in internal/guestagent:
Read at ... by goroutine 433:
testing.(*common).log()
guestagent.(*EgressForwarder).logf() egress.go:52
guestagent.(*EgressForwarder).handle() egress.go:61
Previous write at ... by goroutine 110:
testing.tRunner.func1() testing.go:2023
tRunner.func1 is the test finishing, so a forwarder goroutine was calling t.Logf on a
*testing.T that had already returned. startForwarder passes t.Logf straight to the forwarder;
its t.Cleanup closes the listener, which ends Serve, but Serve spawns each connection with a
bare go f.handle(c) and nothing joins those goroutines.
It predates this branch — main's own egress_test.go, untouched by #620 — and it is fixed
separately in #645 rather than here, since it has nothing to do with test users.
Two things I got wrong on the way to that, worth recording because they are the reason the first
answer was confident and incorrect:
- I called it a flake before reading the log. I could not read the job log (403 without admin
rights) and inferred from the step name that it was the same-racetiming noise as the
supervisorpair. The log said otherwise on the first line of the trace. - My first attempt to verify the fix locally reported "5 runs, 0 races" and meant nothing: this
sandbox forbids Unix socketbind, so every affected test had already failed atbindbefore
reaching the racing code. The one TCP-only test on the same path passes 20 consecutive-race
runs both with and without the fix, so the race needs CI's timing. That fix is reasoned from the
stack trace, and CI is its test — stated as such in its own PR rather than claimed as verified.
Nothing about this changes #620's own verification: control-plane is 1010/1010 on this head,
k8s-kind-e2e is green, and the deploy-scripts, docs and lint jobs pass. The check job's
sbx-push timeout is gone since the #642 rebase. Once #645 lands and this branch is rebased
onto it, proto should be green too — and at that point every red check on this PR will have had a
cause found rather than a re-run.
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
A correction to my previous comment, before it misleads anyone reading back.
I wrote that the only failure left on this head was the supervisor handoff.integration flake.
That was wrong. proto was also red, and not from a flake — go test -race ./... was reporting
a genuine data race in internal/guestagent:
Read at ... by goroutine 433:
testing.(*common).log()
guestagent.(*EgressForwarder).logf() egress.go:52
guestagent.(*EgressForwarder).handle() egress.go:61
Previous write at ... by goroutine 110:
testing.tRunner.func1() testing.go:2023
tRunner.func1 is the test finishing, so a forwarder goroutine was calling t.Logf on a
*testing.T that had already returned. startForwarder passes t.Logf straight to the forwarder;
its t.Cleanup closes the listener, which ends Serve, but Serve spawns each connection with a
bare go f.handle(c) and nothing joins those goroutines.
It predates this branch — main's own egress_test.go, untouched by #620 — and it is fixed
separately in #645 rather than here, since it has nothing to do with test users.
Two things I got wrong on the way to that, worth recording because they are the reason the first
answer was confident and incorrect:
- I called it a flake before reading the log. I could not read the job log (403 without admin
rights) and inferred from the step name that it was the same-racetiming noise as the
supervisorpair. The log said otherwise on the first line of the trace. - My first attempt to verify the fix locally reported "5 runs, 0 races" and meant nothing: this
sandbox forbids Unix socketbind, so every affected test had already failed atbindbefore
reaching the racing code. The one TCP-only test on the same path passes 20 consecutive-race
runs both with and without the fix, so the race needs CI's timing. The fix therefore went out
reasoned from the stack trace rather than claimed as verified — and #645 is now green on all 15
checks,protoincluded, which is the evidence I could not produce locally.
Nothing about this changes #620's own verification: control-plane is 1010/1010 on this head,
k8s-kind-e2e is green, and the deploy-scripts, docs and lint jobs pass. The check job's
sbx-push timeout is gone since the #642 rebase. Once #645 lands and this branch is rebased onto
it, proto will be green here too — it already is on #645. At that point every red check this PR
has had will have had a cause found and fixed, rather than a re-run.
Summary
Every login to the control plane is GitHub's device flow, which needs a person with a browser. smoke.sh works around that by minting an API token inside the control-plane pod with the signing key, which skips refresh.issue(), rolesFor() and the audit login entry -- so the smoke authenticates in a shape no real user has, and needs kubectl exec to do it. Multi-user behaviour (per-subject credentials, cross-user session denial) had no automated coverage above one user.
With SH_TEST_USERS=N the control plane now serves logins for N synthetic subjects test:1..test:N via POST /v1/auth/test-token, and
mocactl login --test-user Ndrives it. A test user is configured exactly like a GitHub user: the handler makes the same two calls as completeDeviceAuth -- refresh.issue() then loginBody() -- so token TTLs, rotation, roles, ownership, audit and credential storage cannot tell them apart. Only the way identity is established differs.The path is gated by SH_TEST_USER_SECRET, which the client presents and the control plane compares in constant time (both sides hashed to 32 bytes first, so a length mismatch is neither an exception nor an oracle). The endpoint issues a real login for an arbitrary subject, so without a secret anyone who could route to the control plane would be every test user -- including a compromised sandbox. The secret is a credential FILE via CONTROL_PLANE_SECRETS, never an env line a kubectl child inherits, and setup.sh generates it into moca-mu1 without rotating it on a re-run.
Fails closed: 404 when SH_TEST_USERS is unset (indistinguishable from an unrouted path), the route is not mounted on the sandbox profile at all and the control plane refuses to boot if the two are combined, N is capped at 1000, the secret must be at least 16 characters, and a warning names the count at boot and on every setup run.
deploy/k8s/test-users.py drives N users (default 10) through real mocactl commands, one XDG_CONFIG_HOME each -- auth.json holds exactly one login, so one directory cannot hold ten -- and asserts the cross-user claims: user 2 is refused user 1's real session, user 1 still resumes it, and each list holds only its owner's sessions. --api runs the same claims over HTTP as a reference for a non-CLI client.
Fixes #616
What this does
With
SH_TEST_USERS=Nthe control plane serves logins for N synthetic subjectstest:1…test:Nvia
POST /v1/auth/test-token, andmocactl login --test-user Ndrives it. Unset, the path doesnot exist.
The decision that carries the value: a test user is configured exactly like a GitHub user. The
handler makes the same two calls as
completeDeviceAuth—deps.refresh.issue()thenloginBody()— so token TTLs, refresh rotation and renewal,rolesFor, the ownership index, theaudit trail and credential storage cannot tell them apart. Only the way identity is established
differs.
Decision record: ADR-0045.
Security: the shared secret
The issue as filed had the endpoint gated only by
SH_TEST_USERS. On review that is not enough, sothis PR adds
SH_TEST_USER_SECRET— required whenever test users are on. The endpoint issues areal login for an arbitrary subject; without a secret, anyone who can route to the control plane
is every test user, including a compromised sandbox on the same cluster. Test clusters are reachable
from more places than their operators assume, which is the common theme in the write-ups on test
environments becoming the breach path.
What the gate actually does:
crypto.timingSafeEqual, with both sides SHA-256'd to 32 bytes first so a length mismatch is neither an exception (it throws on unequal buffers) nor a length oraclesetup.shgenerates 64 hex charsCONTROL_PLANE_SECRETS, so it never entersprocess.env, which everykubectlchild inheritsmocactlreads it fromMOCA_TEST_USER_SECRET, never a flag — a flag would land in shell history and inpsunauthorizedcode for missing and wrong alikeindex, so a bad index and a good one answer identically to an unauthenticated caller404 not_foundwhenSH_TEST_USERSis unset — indistinguishable from an unrouted pathHARNESS, notBOTH), and a boot refusal if the two are combined (SBX0 §4.2 admits only enrolledgithub:subjects)Ncapped at 1000;SH_TEST_USERS=tenis refused rather than coerced to 0setup.shrunsetup.shkeeps the stored value, like every othermoca-mu1secretAccepted costs are written down in the ADR rather than left implicit: a cluster with the variable
set has a login path needing no human, and
test:Ncan be named inSH_ADMIN_SUBJECTS(deliberate— it is what makes the admin path testable — and one more reason the secret is not optional).
Best practices this follows
Industry practice for unattended OAuth testing is a mock or local identity provider with predefined
test users — Dex's mock connector,
oauth2-mock-server, MockServer's OAuth2 support. This takes thesame shape (a fixed set of test identities, no browser) but implements it behind the existing
IdentityProviderseam instead of deploying a server, becauseidentity.tsalready anticipates asecond provider ("slice 2 adds a generic
oidcprovider") and running Dex was already weighed andrejected for slice 1. The alternative of a
MOCA_API_TOKENoverride was rejected precisely becauseit would skip the refresh machinery and so would not be configured like a GitHub user.
The driver script
deploy/k8s/test-users.py— PEP 723uv run, stdlib-only souvneeds no PyPI egress.Why a directory per user (the research question in the issue):
mocactlkeeps its login in oneauth.jsonholding exactly oneCachedAuth(packages/mocactl/src/config.ts), resolved fromXDG_CONFIG_HOME. One process in one directory therefore cannot hold ten logins — a secondloginoverwrites the first. The script gives each user its own config dir, which also makes--jobssafe. Amocactl --profileflag would be the nicer long-term answer and is listed asfollow-up, not done here.
Per user:
login --test-user N→doctor→credentials add(secret on stdin) → one turn.Then the cross-user claims — user 2 refused user 1's real session id, user 1 still able to resume
it (proving the id was real and the refusal was about ownership), a never-existing id refused the
same way, and each user's list holding only its own sessions.
Test commands
Verification done
pnpm -r typecheckpackages/control-planesuitedeploy/k8s/tests/setup.test.shpackages/mocactlsuitemain's under identical back-to-back runs (10 on this branch vs 12 onmain) — pre-existing timing-sensitive UI/sbx tests that flake under parallel load, not regressionsscripts/check-doc-links.pyFour repo guards caught the change and were updated rather than worked around, which is them doing
their job: the OpenAPI contract test (route + error code must be documented), the route inventory and
route-profile allow-lists, the
mocactlclient-coverage test ("adding a new public method must makethis test fail"), and the
moca-settingskey inventory (nine keys → ten).New tests worth naming: the 404 holds for every body shape when the feature is off; a wrong secret
answers identically for a valid and an absurd index;
secretMatchesdoes not throw at any length;the issued refresh token actually rotates;
SH_TEST_USERS+ sandbox profile refuses to boot; thegenerated secret is not a copy of the exchange token (an early version of my
setup.shchangereused
generated_value SH_EXCHANGE_TOKEN, which reads the generator's output by key name and wouldhave made the two identical — the test now pins it).
Live run on OpenShift (
avek2, N=10)Image
quay.io/aslomnet/moca:test-users-3816b706(amd64, built from this commit; the feature wasverified present inside the image, not just assumed). Stack brought up with
SH_TEST_USERS=10 deploy/k8s/setup.sh --target ocp-single, which printed the warning and generateda 64-hex-character secret into
moca-mu1. Six pods, all Running, 0 restarts throughout.The gate, checked before the feature: on the previous image (no
SH_TEST_USERS)POST /v1/auth/test-tokenanswered 404, andmocactl login --test-user 1against it printedthis control plane has no test users (set SH_TEST_USERS on the control plane to enable them).Security checks against the live stack:
401 unauthorized— "wrong or missing secret"index: 1401index: 99999401— identical, so probing cannot count the test usersindexof0/11/1.5/"1"400 invalid_request— "must be a whole number in 1..10"index: 1200:test:1,Test User 1,roles: [], API token and anmrt_…refresh tokenBoth driver modes passed:
Cross-user claims, live: user 2 refused user 1's real session id; user 1 still resumed it; a
never-existing id refused the same way; user 1's list excluding user 2's.
Independent evidence (not the script's own word): the audit stream holds all ten distinct
test:Nsubjects across 92 entries, each with its own credential name (test-user-1…test-user-10), and zero occurrences of the model token's prefix — names recorded, values never.A turn as
test:7ran in a sandbox and repliedmoca-sandbox-0, andmocactl doctorshowed all 7checks green.
The device flow is untouched: a plain
mocactl loginon the same stack printed a fresh GitHubdevice code.
Regression: the existing smoke still passes
deploy/k8s/smoke.sh --target ocp-singleon the test-users image:PASS=12 FAIL=0, all twelveclaims green (claim 7's kube-API
noteis the documented README §12.3 behaviour, not a failure).So enabling test users changes nothing for the existing login, turn, drain, persistence and
isolation paths.
One operational gotcha found while getting there: the smoke binds
18090for the control plane andreaches the harness at the URL the control plane advertises (
127.0.0.1:8080). A leftoverport-forwardon either port makes the smoke's own forward fail to bind(
bind: address already in usein itspf-cp.log) and every/turnanswers 404 — which looksexactly like a broken harness. Stop any forward on
8080/18090first; §9.3 gives the samewarning for its connection count, and §13 now repeats it.
Not done / remaining work
smoke.shstill mints in-pod. Migrating it totest:1is the natural first consumer and thechange that would retire the workaround this PR exists to replace. Deliberately a follow-up so
this PR does not also rewrite the smoke.
mocactl --profile. The driver works around one-login-per-directory withXDG_CONFIG_HOME; a profile flag would be cleaner.--apicreates sessions rather than runningturns, so it does not exercise the harness; the CLI mode does.
Proposed, matching ADR-0039's status.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com