docs(spec): close four MU1 gaps that would make an implementation plan guess - #243
Merged
pdettori merged 1 commit intoSep 9, 2026
Merged
Conversation
…n guess Auditing MU1 for whether a fresh planning session could work from the spec alone found four things underspecified. All four are decisions rather than details, so they belong in the reviewed spec rather than improvised inside a gitignored plan. SH_REQUIRE_AUTH, default off (§4.3.1). "POST /turn requires a session token" could not mean "always": 14 scripts in deploy/knative/ call /turn or /runs unauthenticated today, including setup-kind.sh, setup-ocp.sh, smoke.sh and both demos. Making the token mandatory in one step breaks every smoke path at once. The flag governs whether auth is REQUIRED, never whether it is ENFORCED — a present-but-invalid token is 401 in either mode, because "unauthenticated is allowed here" and "this bad token is close enough" are different statements and only the first is a deployment choice. The demo runs with the flag on, so it demonstrates the real property. §8.1 now states the table assumes it. Exchange authentication (§5.3.1). Three places said "mTLS" without saying where certificates come from — a non-trivial build on kind or on OCP without SPIRE, for a hop with two operator-deployed participants in one cluster. Replaced with the pattern already in the tree: a shared bearer token from a Secret, fail-closed on mismatch, exactly as relay↔worker use SH_RELAY_TOKEN. mTLS stays the target, and Z1 is what makes it cheap — it buys a per-session caller identity a shared secret cannot express. Public-key distribution (§5.2). The spec said the harness holds "only the public key" without saying how it arrives. Now a kid in the JWT header plus SH_SESSION_TOKEN_PUBLIC_KEYS carrying a LIST, so rotation overlaps two keys instead of needing a flag day. Deliberately not a JWKS endpoint: fetching keys at verify time would put a control-plane round trip on every turn and undo §9.2's property that an outage does not break running work. Demo prerequisites (§5.1.1). A shell script cannot complete a browser redirect, so MU1 uses GitHub's device flow — which needs an OAuth app with device flow explicitly enabled, SH_GITHUB_CLIENT_ID, and two real GitHub accounts, none of which a script can create. No client secret is needed, since the device flow treats the app as a public client. The browser authorization-code flow is MU2, with the web UI that needs it. Also refreshes the traced-at note: re-verified at 1deacac after P5 merged, which changed no code and so shifted no line number. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Follow-up to #238 (merged). Before spawning a fresh session to write the MU1 implementation plan, I audited whether the spec + ADR are self-sufficient. Four things were underspecified — and each is a decision, not a detail, so it belongs in the reviewed spec rather than getting improvised inside a gitignored plan.
Docs only. No code changes.
1.
SH_REQUIRE_AUTH, default off (new §4.3.1)The spec said
POST /turn"requires a session token". That can't mean always: 14 scripts indeploy/knative/call/turnor/runsunauthenticated today —setup-kind.sh,setup-ocp.sh,smoke.sh,leaf-smoke.sh,leaf-async-smoke.sh,leaf-gate-smoke.sh,turn-stream-smoke.sh,lib.sh,e6-saturation.sh, and both demos. Making the token mandatory in one step breaks every smoke path and both demos at once — the kind of change that gets reverted rather than fixed.SH_REQUIRE_AUTHfalse(default)truetoken_requiredThe load-bearing detail is the last column: a present-but-bad token fails in either mode. The flag governs whether auth is required, never whether it is enforced — "unauthenticated is allowed here" and "this bad token is close enough" are different statements, and only the first is a deployment choice. The demo runs with the flag on, so what it demonstrates is the real property rather than the permissive default.
§8.1 now opens by stating the whole guarantee table assumes the flag is on, and that under the default the Session drive row does not hold (no token ⇒ no
sidto bind). Flipping the default and updating those 14 callers is MU2.2. Exchange authentication (new §5.3.1)
Three places said the credential exchange happens "over mTLS" and none said where certificates come from. On kind, or on OCP without SPIRE, that's cert issuance + rotation + trust distribution — for a hop with exactly two operator-deployed participants inside one cluster.
Replaced with the pattern already in this tree: a shared bearer token from a Secret, fail-closed on mismatch, exactly how relay↔worker authenticate via
SH_RELAY_TOKEN(relay-deployment.yaml:45,worker-deployment.yaml:35-43). Here it'sSH_EXCHANGE_TOKEN.mTLS stays the stated target, and the spec now says what makes it worth the cost later: Z1's per-session SVIDs identify the caller per session, which a shared secret cannot express. The interim weakness is named — any code running in the harness pod can call the exchange — and bounded: the exchange returns only the subject named in a signed token the harness cannot mint.
3. Public-key distribution and rotation (§5.2)
The spec said the harness holds "only the public key" without saying how it gets there — which also determines how rotation works. Now: a
kidin the JWT header, andSH_SESSION_TOKEN_PUBLIC_KEYScarrying a list of<kid>:<base64 key>, so rotation overlaps two keys instead of needing a flag day. Plain config, not a Secret mount — a public key isn't secret.Deliberately not a JWKS endpoint: fetching keys at verify time would put a control-plane round trip on the critical path of every turn and undo §9.2's property that an IdP or control-plane outage doesn't break running work. Verification stays local arithmetic; the cost is that rotation is a config roll.
4. Demo prerequisites (new §5.1.1)
demo-multiuser.shis a slice-1 deliverable and a shell script — it cannot complete a browser redirect. So MU1 uses GitHub's device flow, and the prerequisites are now written down because none of them can be scripted:SH_GITHUB_CLIENT_ID— and no client secret, since the device flow treats the app as a public client (which is also why it's safe to run from a script a developer reads)The browser authorization-code flow — which does need a client secret and a redirect URI — is MU2, with the web UI that needs it. The script skips with a clear message when
SH_GITHUB_CLIENT_IDis unset, matching the existing env-gated live smokes.ADR-0033
Amended (still
Proposed, so not yet immutable): the mTLS claim corrected, and two accepted costs added — the default-off flag, and the shared-secret hop.Also
Traced-at note refreshed: re-verified at
1deacacafter P5 merged, which changed no code and so shifted no line number.Verification
make lintpasses (prettier, shellcheck, hadolint, gitleaks, yaml). No code touched. Checked that §5.2's JWKS rejection and §9.2's local-verification claim now reinforce rather than contradict each other, and that §4.3.1's table and §9.1's taxonomy agree ontoken_required.🤖 Generated with Claude Code