From a1acbbe60c7a2721ac8fc97a7158e15be82371c1 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Thu, 9 Jul 2026 18:47:23 +0530 Subject: [PATCH] fix(rbac): read hub client id from operator OIDC secret Bootstrap job derived the id as jupyterhub--, but operator releases use other naming conventions, failing the job with "client not found". Read the client-id key from the operator-provisioned -oidc-client Secret instead (the same source the hub mounts at /etc/oauth), polling while the operator is still provisioning. Explicit rbac.bootstrap.hubClientId still wins and skips the read. The job gets a ServiceAccount plus a get-one-secret Role in the release namespace since it runs in the keycloak namespace. Closes #135 --- files/keycloak_rbac_bootstrap.py | 129 ++++++++++++++++- templates/keycloak-rbac-bootstrap-job.yaml | 81 ++++++++++- tests/unit/test_keycloak_rbac_bootstrap.py | 156 +++++++++++++++++++++ values.yaml | 15 +- 4 files changed, 370 insertions(+), 11 deletions(-) diff --git a/files/keycloak_rbac_bootstrap.py b/files/keycloak_rbac_bootstrap.py index 52d06cb..22b3203 100644 --- a/files/keycloak_rbac_bootstrap.py +++ b/files/keycloak_rbac_bootstrap.py @@ -44,10 +44,12 @@ from __future__ import annotations +import base64 import dataclasses import json import logging import os +import ssl import sys import time import urllib.error @@ -73,6 +75,8 @@ class BootstrapConfig: role_name: str shared_mount_groups: tuple[str, ...] hub_external_url: str + hub_client_secret_name: str = "" + hub_client_secret_namespace: str = "" @classmethod def from_env(cls, env: dict[str, str] | None = None) -> "BootstrapConfig": @@ -82,10 +86,100 @@ def from_env(cls, env: dict[str, str] | None = None) -> "BootstrapConfig": kc_host=env["KC_HOST"], admin_password=env["KC_ADMIN_PASSWORD"], realm=env["REALM"], - hub_client_id=env["HUB_CLIENT_ID"], + hub_client_id=env.get("HUB_CLIENT_ID", ""), role_name=env["ROLE_NAME"], shared_mount_groups=tuple(g for g in raw_groups.split(",") if g), hub_external_url=env.get("HUB_EXTERNAL_URL", "").rstrip("/"), + hub_client_secret_name=env.get("HUB_CLIENT_SECRET_NAME", ""), + hub_client_secret_namespace=env.get( + "HUB_CLIENT_SECRET_NAMESPACE", "" + ), + ) + + +class K8sSecretReader: + """Reads keys out of a Kubernetes Secret via the API server. + + Used to fetch the hub OIDC client-id from the Secret the + nebari-operator provisions next to the NebariApp, so the bootstrap + never has to guess the operator's client-naming convention. + Standard library only, mirroring :py:class:`KCAdmin`. + """ + + SERVICEACCOUNT_DIR = "/var/run/secrets/kubernetes.io/serviceaccount" + + def __init__( + self, + *, + base_url: str, + token: str, + opener: Any = None, + request_timeout: int = 30, + ): + self._base_url = base_url.rstrip("/") + self._token = token + self._opener = opener or urllib.request.build_opener() + self._timeout = request_timeout + + @classmethod + def in_cluster(cls) -> "K8sSecretReader": + """Build a reader from the pod's mounted ServiceAccount + credentials and the standard in-cluster API endpoint.""" + host = os.environ["KUBERNETES_SERVICE_HOST"] + port = os.environ.get("KUBERNETES_SERVICE_PORT", "443") + if ":" in host: # bare IPv6 address + host = f"[{host}]" + with open(f"{cls.SERVICEACCOUNT_DIR}/token") as f: + token = f.read().strip() + context = ssl.create_default_context( + cafile=f"{cls.SERVICEACCOUNT_DIR}/ca.crt" + ) + opener = urllib.request.build_opener( + urllib.request.HTTPSHandler(context=context) + ) + return cls( + base_url=f"https://{host}:{port}", token=token, opener=opener, + ) + + def read_key( + self, + namespace: str, + name: str, + key: str, + *, + retries: int = 60, + sleep_seconds: float = 5.0, + ) -> str: + """The operator writes the Secret asynchronously after the + NebariApp is created, so a fresh install's post-install hook can + race it: poll through 404s instead of failing the Job.""" + url = f"{self._base_url}/api/v1/namespaces/{namespace}/secrets/{name}" + req = urllib.request.Request( + url, headers={"Authorization": f"Bearer {self._token}"}, + ) + for attempt in range(retries): + try: + with self._opener.open(req, timeout=self._timeout) as resp: + secret = json.loads(resp.read()) + data = secret.get("data") or {} + if key not in data: + raise RuntimeError( + f"secret {namespace}/{name} has no {key!r} key; " + f"found keys: {sorted(data)}" + ) + return base64.b64decode(data[key]).decode() + except urllib.error.HTTPError as e: + if e.code != 404: + raise + if attempt < retries - 1: + log.info( + "secret %s/%s not there yet (operator still " + "provisioning?); retrying", namespace, name, + ) + time.sleep(sleep_seconds) + raise RuntimeError( + f"secret {namespace}/{name} never appeared; is the " + "nebari-operator running and reconciling the NebariApp?" ) @@ -471,6 +565,33 @@ def assign_role_to_group( ) +def resolve_hub_client_id( + config: BootstrapConfig, secret_reader: K8sSecretReader +) -> str: + """The clientId the nebari-operator gave the hub OIDC client. + + An explicit ``HUB_CLIENT_ID`` (chart value) always wins. Otherwise + read the id out of the operator-provisioned OIDC client Secret, + the same source of truth the hub itself mounts at ``/etc/oauth``, + so the bootstrap never depends on the operator's client-naming + convention. + """ + if config.hub_client_id: + return config.hub_client_id + if not ( + config.hub_client_secret_name and config.hub_client_secret_namespace + ): + raise RuntimeError( + "no hub client id available: set HUB_CLIENT_ID or both " + "HUB_CLIENT_SECRET_NAME and HUB_CLIENT_SECRET_NAMESPACE" + ) + return secret_reader.read_key( + config.hub_client_secret_namespace, + config.hub_client_secret_name, + "client-id", + ) + + def run(config: BootstrapConfig, kc: KCAdmin) -> None: log.info("==> 1. group-membership mapper on 'groups' scope") kc.ensure_groups_mapper(config.realm) @@ -512,6 +633,12 @@ def main() -> int: except KeyError as missing: log.error("missing required env var: %s", missing) return 2 + secret_reader = ( + None if config.hub_client_id else K8sSecretReader.in_cluster() + ) + hub_client_id = resolve_hub_client_id(config, secret_reader) + log.info("hub OIDC clientId: %s", hub_client_id) + config = dataclasses.replace(config, hub_client_id=hub_client_id) kc = KCAdmin(config.kc_host, config.admin_password) run(config, kc) return 0 diff --git a/templates/keycloak-rbac-bootstrap-job.yaml b/templates/keycloak-rbac-bootstrap-job.yaml index f57d3fb..7d45020 100644 --- a/templates/keycloak-rbac-bootstrap-job.yaml +++ b/templates/keycloak-rbac-bootstrap-job.yaml @@ -26,12 +26,14 @@ The chart deploys fine without RBAC bootstrap pre-configured. */}} {{- if and .Values.nebariapp.enabled .Values.rbac.bootstrap.enabled .Values.rbac.bootstrap.kcAdminCredentialSecret }} {{/* -Auto-derive hubClientId from release + chart name when the deployer -hasn't set one explicitly. Matches the nebari-operator NebariApp -client-id pattern: ``jupyterhub--``. Override via -``rbac.bootstrap.hubClientId`` for non-standard layouts. +The hub OIDC client id is read at runtime from the operator-provisioned +OIDC client Secret (the same Secret the hub mounts at /etc/oauth), so +the Job never has to guess the operator's client-naming convention. +``rbac.bootstrap.hubClientId`` remains as an explicit override that +skips the Secret read entirely. */}} -{{- $hubClientId := .Values.rbac.bootstrap.hubClientId | default (printf "jupyterhub-%s-%s" .Release.Name .Chart.Name) }} +{{- $jobNamespace := .Values.rbac.bootstrap.namespace | default .Release.Namespace }} +{{- $oidcSecretName := .Values.rbac.bootstrap.oidcClientSecretName | default (printf "%s-oidc-client" (include "nebari-data-science-pack.fullname" .)) }} {{- $script := .Files.Get "files/keycloak_rbac_bootstrap.py" }} {{- if not $script }} {{- fail "files/keycloak_rbac_bootstrap.py missing from the chart — repackage with the file included." }} @@ -55,6 +57,66 @@ data: keycloak_rbac_bootstrap.py: | {{ $script | indent 4 }} --- +{{/* +The Job usually runs in the ``keycloak`` namespace (to read the KC +admin Secret via env) while the operator writes the OIDC client Secret +into the release namespace, a cross-namespace read that env +``secretKeyRef`` can't do. The script reads it through the K8s API +instead, so the Job gets a ServiceAccount plus a get-one-secret Role +in the release namespace. +*/}} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ include "nebari-data-science-pack.fullname" . }}-rbac-bootstrap + namespace: {{ $jobNamespace | quote }} + labels: + {{- include "nebari-data-science-pack.labels" . | nindent 4 }} + app.kubernetes.io/component: rbac-bootstrap + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "0" + "helm.sh/hook-delete-policy": before-hook-creation +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: {{ include "nebari-data-science-pack.fullname" . }}-rbac-bootstrap-oidc-read + namespace: {{ .Release.Namespace }} + labels: + {{- include "nebari-data-science-pack.labels" . | nindent 4 }} + app.kubernetes.io/component: rbac-bootstrap + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "0" + "helm.sh/hook-delete-policy": before-hook-creation +rules: + - apiGroups: [""] + resources: ["secrets"] + resourceNames: [{{ $oidcSecretName | quote }}] + verbs: ["get"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: {{ include "nebari-data-science-pack.fullname" . }}-rbac-bootstrap-oidc-read + namespace: {{ .Release.Namespace }} + labels: + {{- include "nebari-data-science-pack.labels" . | nindent 4 }} + app.kubernetes.io/component: rbac-bootstrap + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "0" + "helm.sh/hook-delete-policy": before-hook-creation +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: {{ include "nebari-data-science-pack.fullname" . }}-rbac-bootstrap-oidc-read +subjects: + - kind: ServiceAccount + name: {{ include "nebari-data-science-pack.fullname" . }}-rbac-bootstrap + namespace: {{ $jobNamespace | quote }} +--- apiVersion: batch/v1 kind: Job metadata: @@ -86,6 +148,7 @@ spec: app.kubernetes.io/component: rbac-bootstrap spec: restartPolicy: Never + serviceAccountName: {{ include "nebari-data-science-pack.fullname" . }}-rbac-bootstrap containers: - name: bootstrap image: {{ .Values.rbac.bootstrap.image | quote }} @@ -102,8 +165,14 @@ spec: key: {{ .Values.rbac.bootstrap.kcAdminCredentialSecretKey | default "password" | quote }} - name: REALM value: {{ .Values.rbac.bootstrap.realmName | quote }} + {{- with .Values.rbac.bootstrap.hubClientId }} - name: HUB_CLIENT_ID - value: {{ $hubClientId | quote }} + value: {{ . | quote }} + {{- end }} + - name: HUB_CLIENT_SECRET_NAME + value: {{ $oidcSecretName | quote }} + - name: HUB_CLIENT_SECRET_NAMESPACE + value: {{ .Release.Namespace | quote }} - name: ROLE_NAME value: {{ .Values.rbac.bootstrap.sharedMountRoleName | quote }} - name: SHARED_MOUNT_GROUPS diff --git a/tests/unit/test_keycloak_rbac_bootstrap.py b/tests/unit/test_keycloak_rbac_bootstrap.py index 9333a22..749f4d8 100644 --- a/tests/unit/test_keycloak_rbac_bootstrap.py +++ b/tests/unit/test_keycloak_rbac_bootstrap.py @@ -438,6 +438,162 @@ def respond(method, path, **kw): assert "baseUrl" not in client or client["baseUrl"] is None +# --------------------------------------------------------------------------- +# Hub client-id resolution from the operator-provisioned OIDC Secret +# --------------------------------------------------------------------------- + +import base64 +import json + + +class FakeResponse: + def __init__(self, payload: bytes): + self._payload = payload + + def read(self) -> bytes: + return self._payload + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + +class FakeOpener: + """Serves a scripted sequence of responses; an Exception entry is + raised instead of returned.""" + + def __init__(self, responses): + self._responses = list(responses) + self.requests = [] + + def open(self, req, timeout=None): + self.requests.append(req) + item = self._responses.pop(0) + if isinstance(item, Exception): + raise item + return FakeResponse(item) + + +def secret_payload(data: dict[str, str]) -> bytes: + return json.dumps( + {"data": {k: base64.b64encode(v.encode()).decode() for k, v in data.items()}} + ).encode() + + +def test_secret_reader_returns_decoded_client_id(): + opener = FakeOpener([secret_payload({"client-id": "ns-app-client"})]) + reader = rbac.K8sSecretReader( + base_url="https://kube.test", token="sa-token", opener=opener, + ) + value = reader.read_key("data-science", "dsp-oidc-client", "client-id") + assert value == "ns-app-client" + req = opener.requests[0] + assert req.full_url == ( + "https://kube.test/api/v1/namespaces/data-science/secrets/dsp-oidc-client" + ) + assert req.get_header("Authorization") == "Bearer sa-token" + + +def test_secret_reader_polls_until_operator_creates_the_secret(): + """On a fresh install the post-install hook can outrun the operator: + the NebariApp exists but the OIDC Secret hasn't been written yet. + The reader must poll through 404s instead of failing the Job.""" + opener = FakeOpener( + [http_404(), http_404(), secret_payload({"client-id": "late-client"})] + ) + reader = rbac.K8sSecretReader( + base_url="https://kube.test", token="t", opener=opener, + ) + value = reader.read_key( + "data-science", "dsp-oidc-client", "client-id", + retries=5, sleep_seconds=0, + ) + assert value == "late-client" + assert len(opener.requests) == 3 + + +def test_secret_reader_raises_clear_error_when_secret_never_appears(): + opener = FakeOpener([http_404(), http_404()]) + reader = rbac.K8sSecretReader( + base_url="https://kube.test", token="t", opener=opener, + ) + try: + reader.read_key( + "data-science", "dsp-oidc-client", "client-id", + retries=2, sleep_seconds=0, + ) + except RuntimeError as e: + assert "data-science/dsp-oidc-client" in str(e) + else: + raise AssertionError("expected RuntimeError") + + +def test_secret_reader_raises_clear_error_when_key_missing(): + """Secret exists but was provisioned without the expected key + (operator version skew). The error must name the key and secret so + the failure is diagnosable from the Job log alone.""" + opener = FakeOpener([secret_payload({"client-secret": "s3cr3t"})]) + reader = rbac.K8sSecretReader( + base_url="https://kube.test", token="t", opener=opener, + ) + try: + reader.read_key("data-science", "dsp-oidc-client", "client-id") + except RuntimeError as e: + assert "client-id" in str(e) + assert "data-science/dsp-oidc-client" in str(e) + assert "s3cr3t" not in str(e) + else: + raise AssertionError("expected RuntimeError") + + +def test_resolve_hub_client_id_explicit_value_wins_without_k8s_call(): + config = make_config() + reader = MagicMock() + assert rbac.resolve_hub_client_id(config, reader) == HUB_CLIENT_ID + reader.read_key.assert_not_called() + + +def test_resolve_hub_client_id_reads_operator_secret_when_not_explicit(): + config = rbac.BootstrapConfig( + kc_host="http://kc.test", + admin_password="p", + realm=REALM, + hub_client_id="", + role_name=ROLE_NAME, + shared_mount_groups=(), + hub_external_url="", + hub_client_secret_name="dsp-oidc-client", + hub_client_secret_namespace="data-science", + ) + reader = MagicMock() + reader.read_key.return_value = "operator-named-client" + assert rbac.resolve_hub_client_id(config, reader) == "operator-named-client" + reader.read_key.assert_called_once_with( + "data-science", "dsp-oidc-client", "client-id", + ) + + +def test_resolve_hub_client_id_errors_when_nothing_configured(): + config = rbac.BootstrapConfig( + kc_host="http://kc.test", + admin_password="p", + realm=REALM, + hub_client_id="", + role_name=ROLE_NAME, + shared_mount_groups=(), + hub_external_url="", + ) + try: + rbac.resolve_hub_client_id(config, MagicMock()) + except RuntimeError as e: + assert "HUB_CLIENT_ID" in str(e) + assert "HUB_CLIENT_SECRET_NAME" in str(e) + else: + raise AssertionError("expected RuntimeError") + + def test_existing_role_with_wrong_attributes_is_reconciled(): """Deployer (or an older bootstrap) left the role around with the wrong attribute pair. The script must PUT to fix it, not leave the diff --git a/values.yaml b/values.yaml index 2ba3062..e6e5e0c 100644 --- a/values.yaml +++ b/values.yaml @@ -293,11 +293,18 @@ rbac: # Realm to bootstrap. realmName: "nebari" # ClientId of the hub OAuth client in Keycloak (created by the - # nebari-operator NebariApp). Leave empty to derive from the - # release + chart name (the operator's standard pattern: - # ``jupyterhub--``). Override for non-standard - # client ids. + # nebari-operator NebariApp). Leave empty to read it at runtime + # from the operator-provisioned OIDC client Secret (the same + # Secret the hub mounts at /etc/oauth), which tracks whatever + # naming convention the deployed operator uses. Set explicitly + # only to skip the Secret read (e.g. BYO client). hubClientId: "" + # Name of the operator-provisioned OIDC client Secret (in the + # release namespace) the Job reads the clientId from. Leave empty + # to derive ``-oidc-client``, matching the secretName + # the hub mounts under ``jupyterhub.hub.extraVolumes``. Override + # if that mount is overridden. + oidcClientSecretName: "" # Name of the client role the Job creates on the hub OIDC client. # Must match ``KC_SHARED_MOUNT_ROLE`` env var on the hub Deployment # (defaults align).