docs: remove obsolete ServiceAccount example from Entra ID setup#1187
Conversation
|
Hey @cajieh, thanks for the fix, can you please sign the commit to fix the DCO error |
The "With ServiceAccount Credentials" section showed require_oauth=true with cluster_auth_mode="kubeconfig", which the server rejects at startup because all users would share a single cluster identity, breaking per-user audit trails. Remove the invalid OAuth config from the example and add a note explaining the constraint. Fixes openshift#328 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: cyril-ui-developer <cyril.ajieh@gmail.com>
3913745 to
880ec0f
Compare
@nader-ziada Done, thank you. |
manusa
left a comment
There was a problem hiding this comment.
Thanks for catching this — the example genuinely was broken, and your diagnosis (rejected at startup) is correct. I'd like to take a different approach to the fix, though, because removing only the OAuth lines leaves the example in a worse state than before. Requesting changes.
Root cause (for context)
This section was added in #953 with require_oauth = true + cluster_auth_mode = "kubeconfig", which was valid at the time. #1122 (fix(validation)!: reject kubeconfig auth mode with require_oauth at startup — a breaking change) deliberately made that combination invalid: a single shared ServiceAccount collapses all authenticated users to one cluster identity and breaks per-user audit trails (see the error in pkg/config/config.go). The doc was simply never updated afterward, which is the bug this PR is addressing.
Why removing just the OAuth lines isn't the right fix
With require_oauth gone, the example now contains no Entra ID / OAuth configuration at all — yet it lives in the Entra ID setup guide. More importantly, this guide targets HTTP mode throughout (OAuth, MCP Inspector redirect URIs, /.well-known discovery), and in HTTP mode require_oauth = false leaves the MCP endpoint unauthenticated. From the middleware contract in pkg/http/authorization.go: "requireOAuth is false: the OAuth flow is skipped, and the server is effectively unprotected."
So the revised example turns a fail-closed misconfiguration (server refuses to start) into a fail-open one — a reader following an authentication guide would deploy an open, ServiceAccount-privileged HTTP endpoint, with nothing in the section warning them.
The scenario is already covered
This section's premise — "your cluster doesn't accept Entra ID tokens on the API server" — is exactly what the next section, With Token Exchange (On-Behalf-Of Flow), solves in the supported, identity-preserving way (it exchanges the user's Entra token for a downstream token the cluster accepts). The ServiceAccount approach is obsolete post-#1122.
Requested change
Please remove the ### With ServiceAccount Credentials section entirely, including its #### Creating a ServiceAccount Kubeconfig subsection — i.e. everything from ### With ServiceAccount Credentials up to (but not including) ### With Token Exchange (On-Behalf-Of Flow). Optionally, add a one-line pointer telling readers whose clusters don't accept Entra ID tokens to use On-Behalf-Of token exchange.
PR metadata
- The
Fixes #328reference is incorrect — #328 is an unrelated, already-merged PR (test(mcp): refactor events toolset tests). The change this follows from is #1122. Please reference that instead in both the commit message and the PR description (e.g. "Follow-up to #1122" / "Refs #1122" — there's no tracking issue to "Fix"). - Please also update the PR title/description to reflect the new approach (removing the obsolete section, rather than "fixing the unsupported combination").
The "With ServiceAccount Credentials" section combined require_oauth=true with cluster_auth_mode="kubeconfig" — a combination that containers#1122 rejects at startup, since a shared ServiceAccount collapses all authenticated users to one cluster identity and breaks per-user audit trails. Removing only the OAuth lines would leave an unauthenticated, ServiceAccount-privileged HTTP endpoint in an authentication guide. Remove the obsolete section entirely and point readers whose clusters don't accept Entra ID tokens directly to the supported On-Behalf-Of token exchange flow. Refs containers#1122 Signed-off-by: Marc Nuri <marc@marcnuri.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed a maintainer commit to take this over the line. Instead of dropping only the OAuth lines, I removed the Rationale: the example combined I also updated the title/description: the original Thanks @cajieh for catching the broken example! |
The "With ServiceAccount Credentials" section in the Entra ID guide showed
require_oauth = truewithcluster_auth_mode = "kubeconfig"— a combination #1122 deliberately rejects at startup (a shared ServiceAccount collapses all authenticated users to one cluster identity, breaking per-user audit trails).Removing only the OAuth lines would leave an unauthenticated, ServiceAccount-privileged HTTP endpoint in an authentication guide. This PR instead removes the obsolete section entirely (including the "Creating a ServiceAccount Kubeconfig" subsection) and adds a note pointing readers whose clusters don't accept Entra ID tokens directly to the supported On-Behalf-Of token exchange flow, which preserves per-user identity.
Refs #1122