fix(oidc): allow empty issuer to skip discovery for split-horizon#281
Open
devantler wants to merge 7 commits into
Open
fix(oidc): allow empty issuer to skip discovery for split-horizon#281devantler wants to merge 7 commits into
devantler wants to merge 7 commits into
Conversation
Drop the hardcoded http://localhost:8080/realms/crossview issuer default in both the Helm chart (templates/configmap.yaml) and the Go server (lib/sso_config.go) so an unset issuer stays empty. An empty issuer makes the server skip OIDC discovery and use the explicit authorizationURL/tokenURL/userInfoURL verbatim, which is required for split-horizon setups (public authorize URL for the browser + in-cluster token/userinfo). A non-empty issuer still triggers discovery as before. Fixes crossplane-contrib#280 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.com>
…ency firstNonEmpty(env, viper, "") matches the sibling empty-default fields (ClientSecret, AuthorizationURL, TokenURL, UserInfoURL); behaviour is identical. Tightened the guard comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.com>
Keep config/loader.js's OIDC issuer default in sync with the Go server (lib/sso_config.go): an unset issuer stays empty so discovery is skipped. Behaviour-preserving — the loader's sso block is only consumed via getConfig('vite') today — but avoids leaving the same localhost default we removed elsewhere. See crossplane-contrib#280.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.com>
The full split-horizon rationale lives in lib/sso_config.go (the consumed OIDC path); loader.js's sso block is only read via getConfig('vite'), so a one-line pointer is enough here. No code change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.com>
Author
|
@MoeidHeidari should be ready for review now :-) |
The deprecated quintush/helm-unittest fork no longer ships its `untt` binary, so `helm plugin install ... || true` silently failed and `helm unittest` errored with "fork/exec .../untt: no such file or directory". Switch to the maintained helm-unittest/helm-unittest plugin pinned to v1.0.1 — an unpinned install checks out main HEAD whose plugin.yaml uses a `platformHooks` field Helm 3.13.0 cannot parse — and drop `|| true` so a failed install fails the job loudly. Verified locally on Helm 3.13.0: 5 suites / 23 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.com>
Note in the workflow that v1.0.1 is the newest helm-unittest release compatible with the pinned Helm 3.13.0 (v1.1.1+ use a platformHooks plugin.yaml field 3.13 can't parse); bumping the plugin further requires bumping Helm — left to the maintainers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.com>
Author
|
@MoeidHeidari I fixed the failing CI check caused by the use of the deprecated quintush/helm-unittest fork. https://github.com/helm-unittest/helm-unittest seems to be community chosen actively maintained alternative. |
Collaborator
|
thanks @devantler . Even though we usually don't accept AI-generated code, this PR seems fairly straightforward and unambiguous. We can accept it. But before moving forward, we need some adjustments
|
Remove the explanatory code comments (and crossplane-contrib#280 references) flagged in review; the split-horizon behaviour stays documented in docs/SSO_SETUP.md. Keep a one-line note on the helm-unittest v1.0.1 pin (non-obvious: v1.1.1+ need a newer Helm). values.yaml reverts to its original commented example. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Nikolai Emil Damm <nikolaiemildamm@icloud.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.
Fixes #280
Problem
Split-horizon OIDC — a browser-facing
authorizationURLplus in-clustertokenURL/userInfoURLfor the server-side code→token exchange and userinfo lookup — can't be configured. The blocker is a hardcodedhttp://localhost:8080/realms/crossviewissuer default in two places, which forces a non-empty issuer and makes the server run OIDC discovery (whose discovered endpoints then override the explicit ones):templates/configmap.yamlrenderedOIDC_ISSUERwith| default "http://localhost:8080/realms/crossview", so an unsetissuercould never render an emptyOIDC_ISSUER.lib/sso_config.goalso re-defaults an emptyOIDC_ISSUERback to the same localhost URL viafirstNonEmpty(…, "http://localhost:8080/realms/crossview").With a non-empty issuer,
services/sso_service.goruns discovery and the discoveredauthorization_endpoint/token_endpoint/userinfo_endpointtake precedence over the explicit overrides, so the in-cluster token/userinfo endpoints get bypassed (the wall #121 hit).Fix (option 1, end-to-end)
Drop the hardcoded localhost issuer default so an unset/empty
issuerstays empty. The server already supports this path — whenIssuer == ""it skips discovery and uses the explicitauthorizationURL/tokenURL/userInfoURLverbatim — this just makes that path reachable from config.helm/crossview/templates/configmap.yamlOIDC_ISSUERnow defaults to""crossview-go-server/lib/sso_config.goconfig/loader.jshelm/crossview/values.yaml,docs/SSO_SETUP.mdhelm/crossview/tests/configmap_test.yamlOIDC_ISSUER; explicit endpoints pass through…/sso/sso_test_helpers.goConfiguring split-horizon after this change:
A non-empty
issuerbehaves exactly as before (discovery runs).Behavior change & the old default
The default
OIDC_ISSUERis now empty instead ofhttp://localhost:8080/realms/crossview. Enabling OIDC without anissuerand without explicit endpoints now fails fast withOIDC authorization URL not configuredinstead of silently probing localhost.The old localhost default existed for the local Keycloak dev/demo, and that use case is unchanged: it's already set explicitly in the example configs (
config/examples/config.yaml.example,config-session-sso.yaml.example) that the demo copies into the docker-compose-mountedconfig/config.yaml. Every documented OIDC-enable flow (docs/CONFIGURATION.md,docs/SSO_SETUP.md, the examples) setsissuerexplicitly, so none relied on the removed fallback. The localhost default now lives only where it belongs — the example/demo configs — instead of being baked into production config resolution (where it caused this bug).CI fix included (pre-existing, unrelated to the OIDC change)
The
Helm Chart Testsjob was failing on this PR before it ran a single chart assertion:.github/workflows/helm-test.ymlinstalled the deprecatedquintush/helm-unittestfork, which no longer ships itsunttbinary, so the maskedhelm plugin install … || truelefthelm unittesterroring withfork/exec …/untt: no such file or directory. This PR toucheshelm/**and was blocked by it, so it also switches the workflow to the maintainedhelm-unittest/helm-unittestplugin pinned tov1.0.1(an unpinned install checks outmainHEAD, whoseplugin.yamluses aplatformHooksfield Helm 3.13 can't parse) and drops|| trueso a failed install fails loudly. Verified on the runner's Helm 3.13.0: 23 tests pass. Happy to split this into its own PR if you'd prefer.Validation
helm lint✓;helm template✓ — default rendersOIDC_ISSUER: "", split-horizon renders the explicit URLs, a setissuerrenders through.helm unittest .✓ — 5 suites, 23 tests (verified on both Helm 4.2.2 and the CI's Helm 3.13.0).go build ./...✓,go vet ./...✓,go test ./...✓;node --check config/loader.js✓;actionlint✓.🤖 Generated with Claude Code (opened on behalf of @devantler).