feat: configure Enterprise SSO for VM deployments - #1116
Conversation
aivong-openhands
left a comment
aivong-openhands
left a comment
There was a problem hiding this comment.
Reviewed against the OpenHands-Cloud Helm chart guidelines. The Keycloak auto-provisioning logic itself is solid, but there's an important configuration-naming problem that will bite self-hosted consumers.
Important - two near-identical values keys (enterpriseSso vs enterpriseSSO)
This PR adds a new key enterpriseSso (lowercase sso) at charts/openhands/values.yaml:117, but the chart already ships enterpriseSSO (capital SSO) at charts/openhands/values.yaml:224 (added in #325). YAML keys are case-sensitive, so these are two independent toggles that differ only by capitalization:
enterpriseSSO.enabled(existing) ->templates/_env.yamluses.Values.enterpriseSSO.enabledto add"enterprise_sso"toOH_WEB_CLIENT_PROVIDERS_CONFIGURED.enterpriseSso.enabled+idpMetadataUrl(new) -> gates the Keycloak SAML IdP auto-provisioning inkeycloak-config-script.yaml.
Having enterpriseSso and enterpriseSSO side by side in the same values.yaml is a serious foot-gun: a consumer who sets one will almost certainly assume they set the other. This runs against the "consumer-friendly configuration / one knob, one effect" principle. I'd strongly suggest consolidating onto the existing enterpriseSSO key (e.g. add displayName and idpMetadataUrl under it) so a single toggle drives both the web-client provider advertisement and the Keycloak auto-config.
Important - consumer split-brain for raw-Helm installs
Because of the two keys, enabling SSO end-to-end via the raw chart currently requires setting both enterpriseSSO.enabled: true (so the provider is advertised to the web client) and enterpriseSso.enabled: true + idpMetadataUrl (so Keycloak is configured). The new README.md section only documents the lowercase enterpriseSso key, so a consumer who follows it will get Keycloak provisioned but the provider may never be advertised to the web client. Please make the documented path a single, complete toggle.
Related: replicated/openhands.yaml wires the new lowercase enterpriseSso block and ENABLE_ENTERPRISE_SSO, but never sets the capital enterpriseSSO, so OH_WEB_CLIENT_PROVIDERS_CONFIGURED won't include enterprise_sso on Replicated installs.
Question - ENABLE_ENTERPRISE_SSO vs OH_WEB_CLIENT_PROVIDERS_CONFIGURED
The button appears to be driven by ENABLE_ENTERPRISE_SSO (presence-checked, set only in the Replicated layer), while OH_WEB_CLIENT_PROVIDERS_CONFIGURED is driven by the capital enterpriseSSO.enabled. Are both needed? Note that for pure-Helm consumers nothing sets ENABLE_ENTERPRISE_SSO, so it would help to clarify which mechanism is authoritative and ensure the raw chart covers it.
Looks good (no action needed)
- Failure-safety is correct: the provisioning runs in a subshell terminated by
|| echo "WARNING...", so an unreachable/invalid metadata URL warns without blocking pod startup, and the internalexit 1only exits the subshell. validateSignatures: "true",trustEmaildocumented, andhideOnLogin: true(valid as a top-level field on the bundled Keycloak - bitnami chart 24.7.5) are all sensible defaults.- The upsert style (existence check -> PUT/POST,
keycloak_api_call) matches the existing realm/IdP/mapper loop vocabulary, and a separate block is justified since the SAML metadata is imported dynamically and can't live in the static realm template. - The Replicated env mapping that renders
truevs an empty string (rather than the literalfalse) correctly matches the app's presence-check semantics and mirrors the existing conditional-literal pattern in this file.
This review was generated by an AI agent (OpenHands) on behalf of the user.
Adds an 'Enterprise SSO (SAML) Authentication' group to the KOTS config screen with a single toggle, enterprise_sso_enabled. When enabled, the HelmChart values map sets ENABLE_ENTERPRISE_SSO=true on the OpenHands application server, which surfaces the 'Connect to Enterprise SSO' button on the login page. The env mapping uses an if/end template that renders 'true' or '': the backend and web-client config injector only presence-check the variable (strip + truthiness), so rendering the literal string 'false' would still enable the feature. Template style mirrors the existing conditional informers in application.yaml and postgres conditional values in openhands.yaml. The toggle only controls UI visibility; operators still need to create the enterprise_sso SAML/OIDC identity provider in the bundled Keycloak (allhands realm) for sign-in to work — documented in help_text. Co-authored-by: openhands <openhands@all-hands.dev>
Extends the Replicated enterprise_sso_authentication group with two optional fields shown when the toggle is on: - Identity Provider Display Name (default 'Company SSO') - SAML Metadata URL (blank = manage Keycloak manually) The HelmChart values map now passes an enterpriseSso block (enabled/displayName/idpMetadataUrl) into the openhands chart. In the chart's keycloak-config init container (the idempotent realm provisioning script that runs on every pod start), a new gated block: - POSTs the metadata URL to Keycloak's identity-provider/import-config endpoint - upserts the enterprise_sso SAML IdP (trustEmail, validate signatures, hidden from the Keycloak login page; reachable via the app's kc_idp_hint=enterprise_sso hint) - upserts the hardcoded identity_provider=enterprise_sso:saml mapper the app uses to detect SAML logins and skip OAuth-only token handling The block runs after realm create/update and inside a subshell, so a bad or unreachable metadata URL logs a clear error and warns without blocking application startup. Defaults are inert (enabled=false, metadata url empty), so existing installs and OSS values render no behavior change. Schema and README updated; shell script verified with sh -n / bash -n. Co-authored-by: openhands <openhands@all-hands.dev>
Correct the Keycloak import contract, consolidate Enterprise SSO values, secure operator input handling, and cover managed provider lifecycle with regression tests.\n\nCo-authored-by: openhands <openhands@all-hands.dev>
b496556 to
90ca0c2
Compare
saurya
changed the title
Keep the focused Keycloak contract job self-contained by using stdlib parsing and source assertions.\n\nCo-authored-by: openhands <openhands@all-hands.dev>
4 participants
Description
Adds Replicated Admin Console configuration for Enterprise SSO (SAML) in VM deployments, including optional automatic provisioning of the
enterprise_ssoidentity provider in the bundled Keycloak realm.What changed
enterpriseSSO, for both login-provider advertising and Keycloak provisioning.ImportConfigrequest and consumes its flat configuration-map response.validateSignature=true, trust-email, and the hardcodedidentity_provider=enterprise_sso:samlmapper.jq, rather than interpolating them into shell source.idpMetadataUrlis blank.Provisioning remains best-effort: invalid or unreachable metadata logs a warning without blocking OpenHands startup.
Helm Chart Checklist
enabled: false, empty metadata URL).Verification
459 passed— repository script tests107 passed— Helm unit tests, including automatic, manual, and disabled SSO modes18 passed— focused Keycloak and SAML contract and render testskeycloak-config.shpassessh -nandbash -nThis PR description was updated by an AI agent (OpenHands) on behalf of the user.