diff --git a/.nebari-operator-version b/.nebari-operator-version index f4b4689..1474d00 100644 --- a/.nebari-operator-version +++ b/.nebari-operator-version @@ -1 +1 @@ -v0.1.0-alpha.20 +v0.2.0 diff --git a/AGENTS.md b/AGENTS.md index b399cad..29d20b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,8 +17,8 @@ Keycloak auth. See [`docs/src/content/docs/architecture.md`](docs/src/content/do | Path | What | Key files | |---|---|---| -| `operator/` | Go operator (controller-runtime) | `api/v1alpha1/app_types.go` (the CRD), `internal/controller/app_controller.go` (reconcile pipeline), `internal/controller/workload.go` (pod spec), `internal/controller/authz.go` (the authz sidecar for Apps with groups, that can idle, or that run an image), `internal/controller/idle.go` (scale to zero: activity polling, the Idle phase, the activator ReferenceGrant), `internal/controller/activator.go` (the page idle Apps serve while they wake), `internal/controller/sources.go` (source volumes and init containers), `internal/controller/imageref.go` (image references and the registry allowlist for `source.type: image`), `internal/controller/routing.go` (NebariApp) | -| `authz/` | Go sidecar: verifies the forwarded access token and enforces `access.groups` (403 otherwise; Apps restrict access by group only); in passthrough mode (unrestricted Apps that can idle) it only proxies. Either way it reports activity for the idle check | `main.go` (env config), `internal/gate/verify.go` (token checks), `internal/gate/proxy.go` (proxy + probes), `internal/gate/activity.go` (`/activity`) | +| `operator/` | Go operator (controller-runtime) | `api/v1alpha1/app_types.go` (the CRD), `internal/controller/app_controller.go` (reconcile pipeline), `internal/controller/workload.go` (pod spec), `internal/controller/authz.go` (the authz sidecar for Apps that can idle or run an image), `internal/controller/idle.go` (scale to zero: activity polling, the Idle phase, the activator ReferenceGrant), `internal/controller/activator.go` (the page idle Apps serve while they wake), `internal/controller/sources.go` (source volumes and init containers), `internal/controller/imageref.go` (image references and the registry allowlist for `source.type: image`), `internal/controller/routing.go` (NebariApp) | +| `authz/` | Go sidecar in front of Apps that can idle and image Apps: proxies every request and reports activity for the idle check. It checks no tokens: `access.groups` are enforced at the gateway (NebariApp `auth.groups`), and it refuses to start if given group settings | `main.go` (env config), `internal/gate/config.go` (env), `internal/gate/proxy.go` (proxy + probes), `internal/gate/activity.go` (`/activity`) | | `api/` | FastAPI REST API over App CRs | `src/nebari_apps_api/main.py` (routes), `auth.py` (JWT + owner/admin checks), `models.py` (request/response models), `cr.py` (model ↔ CR), `images.py` (image allowlist, mirrors `imageref.go`), `k8s.py` (store) | | `mcp/` | FastMCP server, agent tools, resources and prompts at `/mcp` | `src/nebari_apps_mcp/server.py` (tools, `nebari-apps://` resources, prompts), `auth.py` (MCP OAuth: protected resource metadata, JWT verification), `untrusted.py` (fencing app output) | | `ui/` | React + Vite UI on the Nebari design registry | `src/pages/`, `src/components/` (`app-form/`: state, sections), `src/hooks/use-app-actions.ts`, `src/lib/api.ts`, `src/lib/types.ts` (over the generated `crd.gen.ts`) | @@ -63,11 +63,20 @@ helm lint charts/nebari-apps --set clusterDomain=nebari.example.com hand to `api/src/nebari_apps_api/models.py` (`api/tests/test_crd_conformance.py` fails until it is) and `cr.py`, the MCP `launch_app` arguments, `skill/new-nebari-app/references/manifest.md` and `docs/src/content/docs/app-crd-reference.md`. -- **The NebariApp shape is a pinned contract** (nebari-operator `v0.1.0-alpha.20`, pinned once in +- **The NebariApp shape is a pinned contract** (nebari-operator `v0.2.0`, pinned once in `.nebari-operator-version` and mirrored by `NebariOperatorContract` in `routing.go`), guarded by `operator/internal/controller/nebariapp_contract_test.go`. Do not change emitted fields without updating that test and the "NebariApp fields the chart emits" table in `docs/src/content/docs/configuration.md`. +- **The gateway enforces `access.groups`, not the pod.** A restricted App's NebariApp carries + `auth.groups` with `enforceAtGateway: true`, and nebari-operator's SecurityPolicy returns + 403 to non-members. The authz sidecar is passthrough only (activity for idling, the front + port for image Apps); never add token or group checks back to it, and never set + `forwardAccessToken` (app pods do not get the user's token). Groups are full Keycloak paths + matched exactly (`finance` == `/finance`; `/finance` does not admit `/finance/analysts`), + normalized like nebari-operator's `groups.NormalizePath` in `authz.go` (`groupPath`), + `api/src/nebari_apps_api/groups.py` and `ui/src/lib/access.ts` (`groupPath`); change them + together. apps-api matches `api.adminGroups` and `access.managers.groups` the same way. - **Every per-App API route authorizes through `authorized_app`** (owner, a manager listed in `spec.access.managers`, or an `api.adminGroups` member; only the owner and admins may change the manager list). `api/tests/test_authz.py` enumerates the routes and fails when a @@ -75,8 +84,8 @@ helm lint charts/nebari-apps --set clusterDomain=nebari.example.com - **App pods are untrusted user code.** Keep them non-root, capability-free, without a ServiceAccount token, and with resource limits. User input reaches init containers through environment variables, never shell interpolation. The `app` container stays first and - keeps its name (logs default to it); restricted Apps, Apps that can idle and image Apps add the - `authz` sidecar after it. The operator reads idle Apps' activity from the pod, so treat that answer as + keeps its name (logs default to it); Apps that can idle and image Apps add the `authz` + sidecar after it. The operator reads idle Apps' activity from the pod, so treat that answer as untrusted too (bounded, clamped, and a failed check never idles the App). MCP tools and resources return app-controlled text (logs, files, events) only through the fences in `mcp/src/nebari_apps_mcp/untrusted.py`, and resources read through the same apps-api calls as the matching tools, so they share the API's diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 32119a0..efe90bc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,7 +48,7 @@ chart; see the `chart` job for the arguments). | Component | Directory | Checks | |---|---|---| | Operator (Go) | `operator/` | `go vet ./...`, `go build ./...`, `go test ./... -race` (CI also enforces an `internal/` coverage floor, in `.github/workflows/ci.yaml`), `golangci-lint run` (config in `operator/.golangci.yml`), and the envtest suite (below) | -| Authz sidecar (Go) | `authz/` | `go mod tidy -diff`, `go vet ./...`, `go build ./...`, `golangci-lint run` (config in `authz/.golangci.yml`), `go test ./... -race` with an `internal/` coverage floor (in `.github/workflows/ci.yaml`); the tests mint tokens against a fake JWKS | +| Authz sidecar (Go) | `authz/` | `go mod tidy -diff`, `go vet ./...`, `go build ./...`, `golangci-lint run` (config in `authz/.golangci.yml`), `go test ./... -race` with an `internal/` coverage floor (in `.github/workflows/ci.yaml`) | | CRD / deepcopy | `operator/` | Regenerate with `controller-gen` v0.21.0 (commands below) and commit the result; CI fails on drift. | | apps-api | `api/` | `uv lock --check`, `uv sync --frozen --extra dev`, `uv run --frozen --no-sync python -m pytest -q --cov` (fails under `fail_under` in `pyproject.toml`), then `ruff check .`, `ruff format --check .` and `mypy` (strict) through `uv run --frozen --no-sync` | | apps-mcp | `mcp/` | Same as apps-api, plus `uv pip install --no-deps -e ../api` before the tests (they drive the real API in process). | diff --git a/README.md b/README.md index 7837adf..3cff642 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ start again on the next request | apps-api | REST API for CRUD, status, logs, events, analytics and zip/`.html` upload; writes `App` resources (and each App's write-only Secret) for the UI and the MCP server. | `apps-api` | | apps-ui | The apps list as the home page (with a Needs attention banner, a Mine filter and Open links), app detail with logs and events, the launch form, and a Settings page (the UI and API versions, your groups, the cluster's settings, and how to connect an agent). | `apps-ui` | | apps-mcp | MCP server at `/mcp` that lets coding agents launch and manage apps (tools), attach an app's spec, status, logs and events as context (resources), and follow the triage, launch and access-review workflows (prompts), with Keycloak device-flow login. | `apps-mcp` | -| apps-authz | Sidecar in the pods of Apps that list `access.groups` (returns `403` to everyone else) or that can idle (records when the app last served a request). | `apps-authz` | +| apps-authz | Sidecar in the pods of Apps that can idle (records when the app last served a request) and of image Apps (gives them the one front port). It checks no tokens: `access.groups` are enforced at the gateway. | `apps-authz` | Images are published to `ghcr.io/nebari-dev/apps-pack/` and `quay.io/nebari/apps-pack-`. The UI signs in with keycloak-js and the API validates its @@ -49,7 +49,7 @@ be rebranded from chart values without rebuilding the image ## Quick start -Prerequisites: a Nebari cluster running nebari-operator `v0.1.0-alpha.20` or later (the version +Prerequisites: a Nebari cluster running nebari-operator `v0.2.0` or later (the version pinned in [`.nebari-operator-version`](.nebari-operator-version)), Helm 3.8+, and `kubectl` with cluster-admin. The full list is in [Getting started](https://packs.nebari.dev/apps-pack/getting-started/). @@ -104,8 +104,10 @@ The user guide is at [packs.nebari.dev/apps-pack](https://packs.nebari.dev/apps- ## Known limitations -- `access.groups` is enforced in the app pod by the apps-authz sidecar, not - at the gateway (Apps restrict access by Keycloak group only); group changes apply when the user's token refreshes (5 minutes by default). +- Apps restrict access by Keycloak group only, enforced at the gateway (nebari-operator + `v0.2.0` or later). Groups are full paths matched exactly (`finance` is `/finance`, which + does not include `/finance/analysts`); group changes apply when the user's token refreshes + (5 minutes by default). - Users see and manage the Apps they launched or are listed as managers of (`access.managers`); members of `api.adminGroups` manage all of them. Ownership cannot be transferred yet. diff --git a/api/src/nebari_apps_api/auth.py b/api/src/nebari_apps_api/auth.py index 7418d5e..04ebeeb 100644 --- a/api/src/nebari_apps_api/auth.py +++ b/api/src/nebari_apps_api/auth.py @@ -15,6 +15,7 @@ from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer from .config import settings +from .groups import group_paths from .models import Role _bearer = HTTPBearer(auto_error=False) @@ -37,7 +38,7 @@ def is_admin(user: User) -> bool: """ if not settings.auth_enabled: return True - return any(g.lstrip("/") in settings.admin_groups for g in user.groups) + return bool(group_paths(user.groups) & group_paths(settings.admin_groups)) def app_owner(cr: dict[str, Any]) -> str: @@ -60,15 +61,14 @@ def app_managers(cr: dict[str, Any]) -> dict[str, list[str]]: def is_manager(user: User, cr: dict[str, Any]) -> bool: - """Listed in spec.access.managers by username (case-insensitive) or through a group in the caller's verified token (a leading - "/" ignored on both sides, like api.adminGroups). + """Listed in spec.access.managers by username (case-insensitive) or through a group in the caller's verified token, + matched as full group paths like api.adminGroups ("web-team" is "/web-team"; a subgroup does not count). """ managers = app_managers(cr) name = user.username.lower() if name and any(u.lower() == name for u in managers["users"]): return True - groups = {g.lstrip("/") for g in user.groups} - return any(g.lstrip("/") in groups for g in managers["groups"]) + return bool(group_paths(user.groups) & group_paths(managers["groups"])) def app_role(user: User, cr: dict[str, Any]) -> Role: @@ -133,7 +133,10 @@ def validate(self, token: str) -> User: return User( username=claims.get("preferred_username", claims.get("sub", "")), email=claims.get("email", ""), - groups=[g.lstrip("/") for g in claims.get("groups", [])], + # Full group paths ("/finance"), as nebari-operator v0.2.0+ makes + # Keycloak emit them; a client still sending bare names is read as + # the top-level group of that name. + groups=sorted(group_paths(str(g) for g in claims.get("groups") or [])), ) diff --git a/api/src/nebari_apps_api/config.py b/api/src/nebari_apps_api/config.py index 80791eb..6a0f861 100644 --- a/api/src/nebari_apps_api/config.py +++ b/api/src/nebari_apps_api/config.py @@ -37,11 +37,12 @@ class Settings: ) # Keycloak groups whose members may see and manage every App. Everyone - # else sees and manages only the Apps they own (spec.owner). Group names - # are compared without a leading "/" (Keycloak full-path groups). + # else sees and manages only the Apps they own (spec.owner). Entries are + # full group paths ("admin" is "/admin"; a subgroup such as "/team/admin" + # does not count), compared through groups.group_path. admin_groups: list[str] = field( default_factory=lambda: [ - g.strip().lstrip("/") for g in os.environ.get("APPS_ADMIN_GROUPS", "admin").split(",") if g.strip() + g.strip() for g in os.environ.get("APPS_ADMIN_GROUPS", "admin").split(",") if g.strip() ] ) diff --git a/api/src/nebari_apps_api/groups.py b/api/src/nebari_apps_api/groups.py new file mode 100644 index 0000000..1ca96b2 --- /dev/null +++ b/api/src/nebari_apps_api/groups.py @@ -0,0 +1,28 @@ +"""Keycloak group paths, compared the way nebari-operator matches them. + +nebari-operator (v0.2.0+) enforces an App's access.groups at the gateway +against the token's groups claim, which carries full group paths such as +"/finance" or "/parent/child". An entry is the full path of one group: a bare +name is shorthand for the top-level group ("finance" is "/finance"), and a +parent does not include its subgroups ("/finance" does not admit members of +"/finance/analysts" only). apps-api uses the same rule for api.adminGroups +and access.managers.groups, so one group means the same thing everywhere. +""" + +from __future__ import annotations + +from collections.abc import Iterable + + +def group_path(entry: str) -> str: + """The canonical full path of a group entry, as nebari-operator's + groups.NormalizePath computes it: a leading "/", no trailing "/" and no + empty segments. Returns "" for an entry that names no group ("", "/"). + """ + segments = [s for s in entry.strip().split("/") if s] + return "/" + "/".join(segments) if segments else "" + + +def group_paths(entries: Iterable[str]) -> set[str]: + """The canonical paths of entries, without the ones that name no group.""" + return {p for p in (group_path(e) for e in entries) if p} diff --git a/api/src/nebari_apps_api/main.py b/api/src/nebari_apps_api/main.py index 9472f2d..d704ed2 100644 --- a/api/src/nebari_apps_api/main.py +++ b/api/src/nebari_apps_api/main.py @@ -21,6 +21,7 @@ from . import cr as crmod from .auth import User, app_managers, app_role, can_access, can_change_managers, current_user, is_admin from .config import settings +from .groups import group_path from .images import check_image, parse_allowlist from .k8s import READY_TIMEOUT, AppStore, ConflictError, KubernetesAppStore, NotFoundError from .limits import BodySizeLimitMiddleware @@ -563,19 +564,23 @@ def entry(ns: str, name: str) -> dict[str, int]: def _check_access(access: Any) -> None: - """Groups reach the authz sidecar as a comma-separated list, so an entry - may not be blank or contain a comma (the operator refuses such an App; - this says so up front). + """Each group entry must name a Keycloak group. nebari-operator matches + entries as full group paths ("finance" is "/finance") and denies every + request when all of an App's entries name nothing (such as "/"); the + apps-operator refuses such an App, and this says so up front. """ if access is None: return for entry in access.groups: - if not entry.strip() or "," in entry: - raise HTTPException(422, "access.groups: entries must be non-empty and must not contain ','") + if not group_path(entry): + raise HTTPException(422, "access.groups: entries must name a group, such as finance or /parent/child") if access.managers is not None: - for field in ("users", "groups"): - if any(not entry.strip() for entry in getattr(access.managers, field)): - raise HTTPException(422, f"access.managers.{field}: entries must be non-empty") + if any(not entry.strip() for entry in access.managers.users): + raise HTTPException(422, "access.managers.users: entries must be non-empty") + if any(not group_path(entry) for entry in access.managers.groups): + raise HTTPException( + 422, "access.managers.groups: entries must name a group, such as finance or /parent/child" + ) def _check_source(source: Any) -> None: diff --git a/api/src/nebari_apps_api/models.py b/api/src/nebari_apps_api/models.py index b1d794c..e75b392 100644 --- a/api/src/nebari_apps_api/models.py +++ b/api/src/nebari_apps_api/models.py @@ -144,7 +144,7 @@ class AppRuntime(BaseModel): # When set, the app runs as a Python/pixi service: the operator installs # the pixi environment and executes `pixi run `. The task must # start a server on $PORT (8080, or 8081 when the sidecar fronts it: - # restricted apps and apps that can idle). + # apps that can idle). pixiTask: str = Field(default="", max_length=64) @@ -159,9 +159,11 @@ class AppAccess(BaseModel): # Unknown keys are ignored on reads: an App stored before the users list was # removed from access may still carry it (the apiserver prunes it on the next write). public: bool = False - # A private app with groups is enforced in the app pod by the authz - # sidecar: a member of any group gets in, everyone else gets 403. Empty: - # any signed-in user. + # A private app with groups is enforced at the gateway (nebari-operator + # auth.groups): a member of any group gets in, every other signed-in user + # gets 403. Entries are full group paths ("finance" is "/finance", which + # does not admit members of "/finance/analysts" only). Empty: any + # signed-in user. groups: list[str] = Field(default_factory=list) # Users and groups who may manage the App like its owner; only the owner # and admins may change this list. diff --git a/api/tests/test_apps.py b/api/tests/test_apps.py index f04aff7..f7906ed 100644 --- a/api/tests/test_apps.py +++ b/api/tests/test_apps.py @@ -524,8 +524,8 @@ def test_access_users_is_refused(client, store): assert "users" not in store.apps[("apps", "docs-site")]["spec"]["access"] -@pytest.mark.parametrize("entry", ["a,b", " ", ""]) -def test_access_group_entries_must_be_single_names(client, store, entry): +@pytest.mark.parametrize("entry", ["/", "//", " ", ""]) +def test_access_group_entries_must_name_a_group(client, store, entry): field = "groups" body = make_app_body() body["access"][field] = [entry] diff --git a/api/tests/test_authz.py b/api/tests/test_authz.py index a4035a6..103a120 100644 --- a/api/tests/test_authz.py +++ b/api/tests/test_authz.py @@ -174,6 +174,19 @@ def test_group_membership_comes_from_the_token(as_user, alice_app): assert as_user(User(username="x", groups=["Dave"])).get(f"{PREFIX}/apps/apps/alices").status_code == 403 +def test_groups_match_as_full_paths(as_user, alice_app): + """Groups match the way nebari-operator matches access.groups: "web-team" + and "/web-team" are the same group, a subgroup or a same-named group under + another parent is not, and neither is a parent of the listed group. + """ + get = lambda u: as_user(u).get(f"{PREFIX}/apps/apps/alices").status_code # noqa: E731 + assert get(User(username="x", groups=["/web-team/"])) == 200 + assert get(User(username="x", groups=["/web-team/frontend"])) == 403 + assert get(User(username="x", groups=["/eng/web-team"])) == 403 + assert get(User(username="x", groups=["/team/admin"])) == 403 + assert get(User(username="x", groups=["/admin"])) == 200 + + NEW_MANAGERS = {"users": ["mallory"], "groups": []} @@ -225,6 +238,9 @@ def test_owner_and_admin_can_change_managers(as_user, alice_app, store, user): def test_blank_manager_entries_rejected(as_user, alice_app): resp = as_user(ALICE).patch(f"{PREFIX}/apps/apps/alices", json=access_with({"users": [" "]})) assert resp.status_code == 422 + resp = as_user(ALICE).patch(f"{PREFIX}/apps/apps/alices", json=access_with({"groups": ["/"]})) + assert resp.status_code == 422 + assert "access.managers.groups" in resp.json()["detail"] def test_manager_of_unowned_app(as_user, store): diff --git a/api/tests/test_jwt.py b/api/tests/test_jwt.py index d23ad8b..613ec9b 100644 --- a/api/tests/test_jwt.py +++ b/api/tests/test_jwt.py @@ -199,5 +199,7 @@ def test_identity_and_groups_from_claims(jwks_client): body = resp.json() assert body["username"] == "alice" assert body["email"] == "a@example.test" - # Keycloak's full-path group names lose the leading slash; /admin makes an admin. - assert set(body["groups"]) == {"admin", "analytics"} + # Groups are reported as full paths (a bare name is the top-level group); + # /admin makes an admin. + assert body["groups"] == ["/admin", "/analytics"] + assert body["admin"] is True diff --git a/authz/go.mod b/authz/go.mod index ac4ea8c..e1684d6 100644 --- a/authz/go.mod +++ b/authz/go.mod @@ -3,10 +3,3 @@ module github.com/nebari-dev/nebari-apps-pack/authz go 1.26.1 toolchain go1.27.3 - -require ( - github.com/coreos/go-oidc/v3 v3.21.0 - github.com/go-jose/go-jose/v4 v4.1.6 -) - -require golang.org/x/oauth2 v0.36.0 // indirect diff --git a/authz/go.sum b/authz/go.sum index d61bea6..e69de29 100644 --- a/authz/go.sum +++ b/authz/go.sum @@ -1,6 +0,0 @@ -github.com/coreos/go-oidc/v3 v3.21.0 h1:wZo4Q9Pum8dYEj0eMUPrqR+kvuGkeUplbLpNCkBqoWM= -github.com/coreos/go-oidc/v3 v3.21.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= -github.com/go-jose/go-jose/v4 v4.1.6 h1:sNRkftxFJxwsILiQukqp5z83QBSmntVwmgRY7AqBDO4= -github.com/go-jose/go-jose/v4 v4.1.6/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= -golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= -golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= diff --git a/authz/internal/gate/config.go b/authz/internal/gate/config.go index f17c84b..ee0e2f7 100644 --- a/authz/internal/gate/config.go +++ b/authz/internal/gate/config.go @@ -1,6 +1,8 @@ // Package gate is the apps-authz sidecar: a reverse proxy in front of an app -// container that lets a request through only when it carries a valid -// Keycloak access token whose groups claim names one of the allowed groups. +// container that records when the app last served a request (for the +// operator's idle check) and gives an image App's own port the pack's one +// front port. It checks no tokens: sign-in and access.groups are enforced at +// the gateway by the NebariApp's SecurityPolicy (nebari-operator v0.2.0+). package gate import ( @@ -16,49 +18,28 @@ type Config struct { // Listen is the proxy's address; the Service and the NetworkPolicy // target it (AUTHZ_LISTEN, default :8080). Listen string - // HealthListen serves /healthz, /readyz and /upstreamz without a token, - // on its own port so the app's own paths are never shadowed + // HealthListen serves /healthz, /readyz, /upstreamz and /activity, on + // its own port so the app's own paths are never shadowed // (AUTHZ_HEALTH_LISTEN, default :8082). HealthListen string // Upstream is the app container, e.g. http://127.0.0.1:8081 // (AUTHZ_UPSTREAM). Upstream *url.URL - // Issuers are the accepted iss values (AUTHZ_ISSUERS, comma-separated): - // Keycloak writes the URL its token endpoint was called at, so both the - // public and the in-cluster realm URL can appear. - Issuers []string - // JWKSURL is the realm's certs endpoint, normally on the in-cluster - // Keycloak URL (AUTHZ_JWKS_URL). - JWKSURL string - // ClientID is the OIDC client the gateway logs users in with - // (AUTHZ_CLIENT_ID). Keycloak puts it in azp; aud holds other clients - // (account, realm-management) unless an audience mapper adds it. - ClientID string - // Groups lets in members of any of these groups (AUTHZ_GROUPS, - // comma-separated). - Groups []string - // GroupsClaim is the top-level claim holding the user's groups - // (AUTHZ_GROUPS_CLAIM, default "groups"). - GroupsClaim string - // Passthrough proxies every request without checking a token - // (AUTHZ_PASSTHROUGH=true). The operator sets it on Apps that restrict - // nobody but may be scaled to zero when idle, where the sidecar only - // records activity. It cannot be combined with groups. - Passthrough bool } +// removedEnforcement are the variables of the enforcing mode this sidecar +// no longer has. An operator older than the sidecar may still set them for +// a restricted App; FromEnv refuses to start then, so the App stays down +// instead of running open to every signed-in user. +var removedEnforcement = []string{"AUTHZ_GROUPS", "AUTHZ_ISSUERS", "AUTHZ_JWKS_URL", "AUTHZ_CLIENT_ID"} + // FromEnv builds a Config from getenv (os.Getenv in main), reporting every -// missing or invalid variable at once. +// missing or invalid variable at once. AUTHZ_PASSTHROUGH, which earlier +// operators set on Apps that only idle, is ignored: every request passes. func FromEnv(getenv func(string) string) (Config, error) { cfg := Config{ Listen: orDefault(getenv("AUTHZ_LISTEN"), ":8080"), HealthListen: orDefault(getenv("AUTHZ_HEALTH_LISTEN"), ":8082"), - Issuers: splitList(getenv("AUTHZ_ISSUERS")), - JWKSURL: strings.TrimSpace(getenv("AUTHZ_JWKS_URL")), - ClientID: strings.TrimSpace(getenv("AUTHZ_CLIENT_ID")), - Groups: splitList(getenv("AUTHZ_GROUPS")), - GroupsClaim: orDefault(getenv("AUTHZ_GROUPS_CLAIM"), "groups"), - Passthrough: strings.TrimSpace(getenv("AUTHZ_PASSTHROUGH")) == "true", } var errs []error @@ -72,28 +53,11 @@ func FromEnv(getenv func(string) string) (Config, error) { if cfg.Listen == cfg.HealthListen { errs = append(errs, errors.New("AUTHZ_LISTEN and AUTHZ_HEALTH_LISTEN must differ")) } - if cfg.Passthrough { - // Refuse rather than silently open an App that meant to restrict. - if len(cfg.Groups) > 0 { - errs = append(errs, errors.New("AUTHZ_PASSTHROUGH cannot be combined with AUTHZ_GROUPS")) + for _, k := range removedEnforcement { + if strings.TrimSpace(getenv(k)) != "" { + errs = append(errs, fmt.Errorf("%s is set, but this sidecar no longer checks tokens or groups: "+ + "access.groups are enforced at the gateway, so upgrade the apps-operator to match this image", k)) } - return cfg, errors.Join(errs...) - } - if len(cfg.Issuers) == 0 { - errs = append(errs, errors.New("AUTHZ_ISSUERS is required (comma-separated realm URLs)")) - } - if cfg.JWKSURL == "" { - errs = append(errs, errors.New("AUTHZ_JWKS_URL is required")) - } else if u, err := url.Parse(cfg.JWKSURL); err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { - errs = append(errs, fmt.Errorf("AUTHZ_JWKS_URL %q must be an http(s) URL", cfg.JWKSURL)) - } - if cfg.ClientID == "" { - errs = append(errs, errors.New("AUTHZ_CLIENT_ID is required")) - } - if len(cfg.Groups) == 0 { - // The sidecar exists only to restrict; with nothing to allow it - // would deny everyone, which is a misconfiguration, not a policy. - errs = append(errs, errors.New("AUTHZ_GROUPS is required (comma-separated)")) } return cfg, errors.Join(errs...) } @@ -104,14 +68,3 @@ func orDefault(v, def string) string { } return def } - -// splitList splits a comma-separated value, dropping blanks. -func splitList(v string) []string { - var out []string - for _, s := range strings.Split(v, ",") { - if s = strings.TrimSpace(s); s != "" { - out = append(out, s) - } - } - return out -} diff --git a/authz/internal/gate/gate_test.go b/authz/internal/gate/gate_test.go index f015db7..dd9afe4 100644 --- a/authz/internal/gate/gate_test.go +++ b/authz/internal/gate/gate_test.go @@ -2,10 +2,6 @@ package gate import ( "context" - "crypto/ecdsa" - "crypto/elliptic" - "crypto/rand" - "crypto/rsa" "encoding/json" "io" "log/slog" @@ -15,253 +11,12 @@ import ( "net/http/httptest" "net/url" "strings" - "sync/atomic" "testing" "time" - - "github.com/coreos/go-oidc/v3/oidc" - jose "github.com/go-jose/go-jose/v4" -) - -const ( - publicIssuer = "https://keycloak.nebari.local/realms/nebari" - internalIssuer = "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080/realms/nebari" - clientID = "apps-app-web-terminal" ) -// realm is a fake Keycloak realm: a signing key, its JWKS endpoint and a -// token minter. -type realm struct { - key *rsa.PrivateKey - kid string - srv *httptest.Server - fetches atomic.Int32 - fail atomic.Bool -} - -func newRealm(t *testing.T) *realm { - t.Helper() - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal(err) - } - enc, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal(err) - } - r := &realm{key: key, kid: "YnxUY0-sig"} - // Like Keycloak's certs endpoint: a signing key and an encryption key. - set := jose.JSONWebKeySet{Keys: []jose.JSONWebKey{ - {Key: &key.PublicKey, KeyID: r.kid, Algorithm: "RS256", Use: "sig"}, - {Key: &enc.PublicKey, KeyID: "OySt7L-enc", Algorithm: "RSA-OAEP", Use: "enc"}, - }} - r.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - r.fetches.Add(1) - if r.fail.Load() { - http.Error(w, "down", http.StatusServiceUnavailable) - return - } - _ = json.NewEncoder(w).Encode(set) - })) - t.Cleanup(r.srv.Close) - return r -} - -func (r *realm) jwksURL() string { return r.srv.URL + "/realms/nebari/protocol/openid-connect/certs" } - -// keycloakClaims is the shape of a real access token from the dev cluster's -// realm for a nebari-operator client (values replaced): azp is the client, -// aud lists other clients, groups are plain names, there is no nbf. -func keycloakClaims(now time.Time) map[string]any { - return map[string]any{ - "exp": now.Add(5 * time.Minute).Unix(), - "iat": now.Unix(), - "auth_time": now.Unix(), - "jti": "onrtac:00000000-0000-0000-0000-000000000000", - "iss": publicIssuer, - "aud": []string{"realm-management", "account"}, - "sub": "00000000-0000-0000-0000-000000000001", - "typ": "Bearer", - "azp": clientID, - "sid": "00000000-0000-0000-0000-000000000002", - "acr": "1", - "allowed-origins": []string{"https://web-terminal.apps.nebari.local"}, - "realm_access": map[string]any{"roles": []string{"offline_access", "default-roles-nebari"}}, - "scope": "openid groups profile email", - "email_verified": true, - "name": "Ada Lovelace", - "groups": []string{"analysts", "test-group"}, - "preferred_username": "ada", - "given_name": "Ada", - "family_name": "Lovelace", - "email": "ada@example.com", - } -} - -func sign(t *testing.T, key any, alg jose.SignatureAlgorithm, kid string, claims map[string]any) string { - t.Helper() - opts := (&jose.SignerOptions{}).WithType("JWT") - if kid != "" { - opts = opts.WithHeader("kid", kid) - } - signer, err := jose.NewSigner(jose.SigningKey{Algorithm: alg, Key: key}, opts) - if err != nil { - t.Fatal(err) - } - payload, err := json.Marshal(claims) - if err != nil { - t.Fatal(err) - } - jws, err := signer.Sign(payload) - if err != nil { - t.Fatal(err) - } - s, err := jws.CompactSerialize() - if err != nil { - t.Fatal(err) - } - return s -} - -func (r *realm) mint(t *testing.T, claims map[string]any) string { - return sign(t, r.key, jose.RS256, r.kid, claims) -} - -func testConfig(r *realm) Config { - return Config{ - Listen: ":8080", - HealthListen: ":8082", - Upstream: &url.URL{Scheme: "http", Host: "127.0.0.1:8081"}, - Issuers: []string{internalIssuer, publicIssuer}, - JWKSURL: r.jwksURL(), - ClientID: clientID, - Groups: []string{"analysts", "admin"}, - GroupsClaim: "groups", - } -} - func quietLog() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } -func with(base map[string]any, edits map[string]any) map[string]any { - out := maps.Clone(base) - for k, v := range edits { - if v == nil { - delete(out, k) - } else { - out[k] = v - } - } - return out -} - -func TestVerify(t *testing.T) { - r := newRealm(t) - now := time.Now() - base := keycloakClaims(now) - other, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal(err) - } - ecKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - if err != nil { - t.Fatal(err) - } - - cases := []struct { - name string - token func() string - cfg func(*Config) - allowed bool - }{ - {name: "member, public issuer", allowed: true, - token: func() string { return r.mint(t, base) }}, - {name: "member, in-cluster issuer", allowed: true, - token: func() string { return r.mint(t, with(base, map[string]any{"iss": internalIssuer})) }}, - {name: "groups claim as a single string", allowed: true, - token: func() string { return r.mint(t, with(base, map[string]any{"groups": "admin"})) }}, - {name: "full group path", allowed: true, - token: func() string { return r.mint(t, with(base, map[string]any{"groups": []string{"/analysts"}})) }}, - {name: "client in aud instead of azp", allowed: true, - token: func() string { - return r.mint(t, with(base, map[string]any{"azp": "other", "aud": []string{"account", clientID}})) - }}, - {name: "nbf within leeway", allowed: true, - token: func() string { return r.mint(t, with(base, map[string]any{"nbf": now.Add(10 * time.Second).Unix()})) }}, - {name: "expired within leeway", allowed: true, - token: func() string { return r.mint(t, with(base, map[string]any{"exp": now.Add(-10 * time.Second).Unix()})) }}, - {name: "non-member", - token: func() string { return r.mint(t, with(base, map[string]any{"groups": []string{"test-group"}})) }}, - {name: "no groups claim", - token: func() string { return r.mint(t, with(base, map[string]any{"groups": nil})) }}, - {name: "groups claim of the wrong type", - token: func() string { return r.mint(t, with(base, map[string]any{"groups": 7})) }}, - {name: "wrong issuer", - token: func() string { - return r.mint(t, with(base, map[string]any{"iss": "https://evil.example/realms/nebari"})) - }}, - {name: "expired", - token: func() string { return r.mint(t, with(base, map[string]any{"exp": now.Add(-time.Minute).Unix()})) }}, - {name: "no exp", - token: func() string { return r.mint(t, with(base, map[string]any{"exp": nil})) }}, - {name: "not valid yet", - token: func() string { return r.mint(t, with(base, map[string]any{"nbf": now.Add(time.Minute).Unix()})) }}, - {name: "bad nbf", - token: func() string { return r.mint(t, with(base, map[string]any{"nbf": "soon"})) }}, - {name: "wrong client", - token: func() string { return r.mint(t, with(base, map[string]any{"azp": "apps-other-app"})) }}, - {name: "bad azp", - token: func() string { return r.mint(t, with(base, map[string]any{"azp": 1})) }}, - {name: "ID token", - token: func() string { return r.mint(t, with(base, map[string]any{"typ": "ID"})) }}, - {name: "bad typ", - token: func() string { return r.mint(t, with(base, map[string]any{"typ": 1})) }}, - {name: "signed by another key", - token: func() string { return sign(t, other, jose.RS256, r.kid, base) }}, - {name: "unknown key id", - token: func() string { return sign(t, other, jose.RS256, "rotated", base) }}, - {name: "symmetric algorithm", - token: func() string { return sign(t, []byte("0123456789abcdef0123456789abcdef"), jose.HS256, r.kid, base) }}, - {name: "ES256 key not in the realm", - token: func() string { return sign(t, ecKey, jose.ES256, "ec", base) }}, - {name: "not a JWT", - token: func() string { return "not.a.jwt" }}, - - {name: "custom groups claim", allowed: true, - cfg: func(c *Config) { c.GroupsClaim = "teams" }, - token: func() string { return r.mint(t, with(base, map[string]any{"groups": nil, "teams": []string{"admin"}})) }}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - cfg := testConfig(r) - if tc.cfg != nil { - tc.cfg(&cfg) - } - v := NewVerifier(cfg, oidc.NewRemoteKeySet(context.Background(), r.jwksURL())) - err := v.Verify(context.Background(), tc.token()) - if tc.allowed && err != nil { - t.Fatalf("denied: %v", err) - } - if !tc.allowed { - if err == nil { - t.Fatal("allowed, want denied") - } - if _, ok := err.(*Denial); !ok { - t.Fatalf("error %T is not a *Denial", err) - } - } - }) - } -} - -func TestVerifyKeySetDownDenies(t *testing.T) { - r := newRealm(t) - r.fail.Store(true) - v := NewVerifier(testConfig(r), oidc.NewRemoteKeySet(context.Background(), r.jwksURL())) - if err := v.Verify(context.Background(), r.mint(t, keycloakClaims(time.Now()))); err == nil { - t.Fatal("a token was allowed while the key set could not be fetched") - } -} - // app is the upstream: it echoes what the proxy sent it. func newApp(t *testing.T) *httptest.Server { t.Helper() @@ -277,54 +32,30 @@ func newApp(t *testing.T) *httptest.Server { return srv } -func newGate(t *testing.T, r *realm, upstream string, edit func(*Config)) (*httptest.Server, Config) { +func newGate(t *testing.T, upstream string) *httptest.Server { t.Helper() - cfg := testConfig(r) u, err := url.Parse(upstream) if err != nil { t.Fatal(err) } - cfg.Upstream = u - if edit != nil { - edit(&cfg) - } - v := NewVerifier(cfg, oidc.NewRemoteKeySet(context.Background(), r.jwksURL())) - srv := httptest.NewServer(NewHandler(cfg, v, http.DefaultTransport, quietLog())) + srv := httptest.NewServer(NewHandler(Config{Upstream: u}, http.DefaultTransport, quietLog())) t.Cleanup(srv.Close) - return srv, cfg + return srv } +// Every request passes, signed in or not: the gateway enforces sign-in and +// access.groups before a request reaches the pod. func TestHandler(t *testing.T) { - r := newRealm(t) - app := newApp(t) - gate, _ := newGate(t, r, app.URL, nil) - now := time.Now() - member := r.mint(t, keycloakClaims(now)) - outsider := r.mint(t, with(keycloakClaims(now), map[string]any{"groups": []string{"test-group"}})) - - cases := []struct { - name string - auth string - status int - body string - }{ - {"member", "Bearer " + member, http.StatusOK, "app:/a/b?c=d"}, - {"lowercase scheme", "bearer " + member, http.StatusOK, "app:/a/b?c=d"}, - {"non-member", "Bearer " + outsider, http.StatusForbidden, "Forbidden: this app is restricted to groups: analysts, admin"}, - {"missing header", "", http.StatusUnauthorized, "Unauthorized: no access token was forwarded to this app"}, - {"basic auth", "Basic YWRhOnB3", http.StatusForbidden, "Forbidden"}, - {"empty bearer", "Bearer ", http.StatusForbidden, "Forbidden"}, - {"garbage", "Bearer garbage", http.StatusForbidden, "Forbidden"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { + gate := newGate(t, newApp(t).URL) + for name, auth := range map[string]string{"no token": "", "bearer": "Bearer abc.def.ghi", "basic": "Basic YWRhOnB3"} { + t.Run(name, func(t *testing.T) { req, _ := http.NewRequest(http.MethodGet, gate.URL+"/a/b?c=d", nil) req.Host = "web-terminal.apps.nebari.local" req.Header.Set("X-Forwarded-For", "203.0.113.7") req.Header.Set("X-Forwarded-Proto", "https") req.Header.Set("X-Forwarded-Host", "web-terminal.apps.nebari.local") - if tc.auth != "" { - req.Header.Set("Authorization", tc.auth) + if auth != "" { + req.Header.Set("Authorization", auth) } resp, err := http.DefaultClient.Do(req) if err != nil { @@ -332,20 +63,11 @@ func TestHandler(t *testing.T) { } defer func() { _ = resp.Body.Close() }() body, _ := io.ReadAll(resp.Body) - if resp.StatusCode != tc.status { - t.Fatalf("status %d, want %d (%s)", resp.StatusCode, tc.status, body) + if resp.StatusCode != http.StatusOK || string(body) != "app:/a/b?c=d" { + t.Fatalf("status %d body %q", resp.StatusCode, body) } - if !strings.HasPrefix(string(body), tc.body) { - t.Fatalf("body %q, want prefix %q", body, tc.body) - } - if strings.Contains(string(body), "eyJ") { - t.Fatal("the response echoes the token") - } - if tc.status != http.StatusOK { - return - } - if got := resp.Header.Get("X-Seen-Authorization"); got != tc.auth { - t.Errorf("upstream Authorization %q, want it passed through", got) + if got := resp.Header.Get("X-Seen-Authorization"); got != auth { + t.Errorf("upstream Authorization %q, want it passed through unchanged", got) } if got := resp.Header.Get("X-Seen-Host"); got != "web-terminal.apps.nebari.local" { t.Errorf("upstream Host %q", got) @@ -361,11 +83,8 @@ func TestHandler(t *testing.T) { } func TestHandlerNoXFF(t *testing.T) { - r := newRealm(t) - gate, _ := newGate(t, r, newApp(t).URL, nil) - req, _ := http.NewRequest(http.MethodGet, gate.URL, nil) - req.Header.Set("Authorization", "Bearer "+r.mint(t, keycloakClaims(time.Now()))) - resp, err := http.DefaultClient.Do(req) + gate := newGate(t, newApp(t).URL) + resp, err := http.Get(gate.URL) if err != nil { t.Fatal(err) } @@ -376,17 +95,14 @@ func TestHandlerNoXFF(t *testing.T) { } func TestHandlerUpstreamDown(t *testing.T) { - r := newRealm(t) ln, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) } addr := ln.Addr().String() _ = ln.Close() - gate, _ := newGate(t, r, "http://"+addr, nil) - req, _ := http.NewRequest(http.MethodGet, gate.URL, nil) - req.Header.Set("Authorization", "Bearer "+r.mint(t, keycloakClaims(time.Now()))) - resp, err := http.DefaultClient.Do(req) + gate := newGate(t, "http://"+addr) + resp, err := http.Get(gate.URL) if err != nil { t.Fatal(err) } @@ -397,7 +113,6 @@ func TestHandlerUpstreamDown(t *testing.T) { } func TestHandlerWebSocketUpgrade(t *testing.T) { - r := newRealm(t) // A raw upstream that accepts the upgrade and echoes one line. up := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.Header.Get("Upgrade") != "websocket" { @@ -416,7 +131,7 @@ func TestHandlerWebSocketUpgrade(t *testing.T) { _ = rw.Flush() })) t.Cleanup(up.Close) - gate, _ := newGate(t, r, up.URL, nil) + gate := newGate(t, up.URL) conn, err := net.Dial("tcp", strings.TrimPrefix(gate.URL, "http://")) if err != nil { @@ -424,8 +139,7 @@ func TestHandlerWebSocketUpgrade(t *testing.T) { } defer func() { _ = conn.Close() }() _ = conn.SetDeadline(time.Now().Add(5 * time.Second)) - _, _ = io.WriteString(conn, "GET /ws HTTP/1.1\r\nHost: app\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n"+ - "Authorization: Bearer "+r.mint(t, keycloakClaims(time.Now()))+"\r\n\r\n") + _, _ = io.WriteString(conn, "GET /ws HTTP/1.1\r\nHost: app\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n\r\n") buf := make([]byte, 4096) n, err := conn.Read(buf) if err != nil || !strings.HasPrefix(string(buf[:n]), "HTTP/1.1 101") { @@ -446,14 +160,13 @@ func TestHandlerWebSocketUpgrade(t *testing.T) { } func TestHealth(t *testing.T) { - ready := false up, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) } defer func() { _ = up.Close() }() d := &net.Dialer{} - h := &Health{Ready: func() bool { return ready }, Upstream: up.Addr().String(), Dial: d.DialContext} + h := &Health{Upstream: up.Addr().String(), Dial: d.DialContext} get := func(path string) int { rec := httptest.NewRecorder() @@ -463,12 +176,8 @@ func TestHealth(t *testing.T) { if c := get("/healthz"); c != 200 { t.Errorf("/healthz %d", c) } - if c := get("/readyz"); c != 503 { - t.Errorf("/readyz before keys %d, want 503", c) - } - ready = true if c := get("/readyz"); c != 200 { - t.Errorf("/readyz after keys %d", c) + t.Errorf("/readyz %d (ready at once: there are no keys to wait for)", c) } if c := get("/upstreamz"); c != 200 { t.Errorf("/upstreamz with a listening app %d", c) @@ -482,90 +191,35 @@ func TestHealth(t *testing.T) { } } -func TestKeyWatch(t *testing.T) { - r := newRealm(t) - r.fail.Store(true) - w := &KeyWatch{URL: r.jwksURL(), Client: r.srv.Client(), Every: 10 * time.Millisecond, Log: quietLog()} - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - done := make(chan struct{}) - go func() { w.Run(ctx); close(done) }() - - deadline := time.Now().Add(5 * time.Second) - for r.fetches.Load() < 3 && time.Now().Before(deadline) { - time.Sleep(5 * time.Millisecond) - } - if w.Ready() { - t.Fatal("ready before a fetch succeeded") - } - r.fail.Store(false) - select { - case <-done: - case <-time.After(5 * time.Second): - t.Fatal("did not become ready") - } - if !w.Ready() { - t.Fatal("not ready after a successful fetch") - } -} - -func TestKeyWatchRejectsBadKeySets(t *testing.T) { - for name, body := range map[string]string{"empty": `{"keys":[]}`, "not json": ``} { - t.Run(name, func(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = io.WriteString(w, body) })) - defer srv.Close() - w := &KeyWatch{URL: srv.URL, Client: srv.Client(), Log: quietLog()} - if err := w.fetch(context.Background()); err == nil { - t.Fatal("accepted") - } - }) - } - w := &KeyWatch{URL: "http://127.0.0.1:1/\x00", Client: http.DefaultClient, Log: quietLog()} - if err := w.fetch(context.Background()); err == nil { - t.Fatal("bad URL accepted") - } - ctx, cancel := context.WithCancel(context.Background()) - cancel() - w = &KeyWatch{URL: "http://127.0.0.1:1/", Client: http.DefaultClient, Every: time.Hour, Log: quietLog()} - w.Run(ctx) // returns on a cancelled context - if w.Ready() { - t.Fatal("ready without keys") - } -} - func env(m map[string]string) func(string) string { return func(k string) string { return m[k] } } func TestFromEnv(t *testing.T) { - full := map[string]string{ - "AUTHZ_UPSTREAM": "http://127.0.0.1:8081", - "AUTHZ_ISSUERS": internalIssuer + ", " + publicIssuer + ",", - "AUTHZ_JWKS_URL": "http://keycloak/realms/nebari/protocol/openid-connect/certs", - "AUTHZ_CLIENT_ID": clientID, - "AUTHZ_GROUPS": "analysts, admin", - } + full := map[string]string{"AUTHZ_UPSTREAM": "http://127.0.0.1:8081"} cfg, err := FromEnv(env(full)) if err != nil { t.Fatal(err) } - if cfg.Listen != ":8080" || cfg.HealthListen != ":8082" || cfg.GroupsClaim != "groups" { + if cfg.Listen != ":8080" || cfg.HealthListen != ":8082" || cfg.Upstream.String() != "http://127.0.0.1:8081" { t.Errorf("defaults: %+v", cfg) } - if len(cfg.Issuers) != 2 || cfg.Issuers[1] != publicIssuer || len(cfg.Groups) != 2 || cfg.Groups[1] != "admin" { - t.Errorf("lists: %+v", cfg) + // Earlier operators set AUTHZ_PASSTHROUGH on Apps that only idle; it is + // ignored now that every request passes. + if _, err := FromEnv(env(map[string]string{"AUTHZ_UPSTREAM": "http://127.0.0.1:8081", "AUTHZ_PASSTHROUGH": "true"})); err != nil { + t.Errorf("AUTHZ_PASSTHROUGH: %v", err) } cases := map[string]struct { edit map[string]string want string }{ - "nothing to allow": {map[string]string{"AUTHZ_GROUPS": " , "}, "AUTHZ_GROUPS is required"}, "no upstream": {map[string]string{"AUTHZ_UPSTREAM": ""}, "AUTHZ_UPSTREAM is required"}, "bad upstream": {map[string]string{"AUTHZ_UPSTREAM": "127.0.0.1:8081"}, "AUTHZ_UPSTREAM"}, - "no issuers": {map[string]string{"AUTHZ_ISSUERS": ""}, "AUTHZ_ISSUERS is required"}, - "no jwks": {map[string]string{"AUTHZ_JWKS_URL": ""}, "AUTHZ_JWKS_URL is required"}, - "bad jwks": {map[string]string{"AUTHZ_JWKS_URL": "keycloak/certs"}, "AUTHZ_JWKS_URL"}, - "no client": {map[string]string{"AUTHZ_CLIENT_ID": ""}, "AUTHZ_CLIENT_ID is required"}, "same listen ports": {map[string]string{"AUTHZ_HEALTH_LISTEN": ":8080"}, "must differ"}, + // An older operator restricting an App: refuse, never run it open. + "groups": {map[string]string{"AUTHZ_GROUPS": "analysts"}, "AUTHZ_GROUPS is set"}, + "issuers": {map[string]string{"AUTHZ_ISSUERS": "https://kc/realms/nebari"}, "AUTHZ_ISSUERS is set"}, + "jwks": {map[string]string{"AUTHZ_JWKS_URL": "https://kc/certs"}, "AUTHZ_JWKS_URL is set"}, + "client id": {map[string]string{"AUTHZ_CLIENT_ID": "apps-app-x"}, "AUTHZ_CLIENT_ID is set"}, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -576,19 +230,15 @@ func TestFromEnv(t *testing.T) { } }) } - if _, err := FromEnv(env(nil)); err == nil || strings.Count(err.Error(), "\n") < 4 { - t.Errorf("an empty environment should report every missing variable: %v", err) + if _, err := FromEnv(env(map[string]string{"AUTHZ_GROUPS": "a", "AUTHZ_HEALTH_LISTEN": ":8080"})); err == nil || strings.Count(err.Error(), "\n") < 2 { + t.Errorf("every problem should be reported at once: %v", err) } } func TestRun(t *testing.T) { - r := newRealm(t) app := newApp(t) - cfg := testConfig(r) u, _ := url.Parse(app.URL) - cfg.Upstream = u - cfg.Listen = freeAddr(t) - cfg.HealthListen = freeAddr(t) + cfg := Config{Upstream: u, Listen: freeAddr(t), HealthListen: freeAddr(t)} ctx, cancel := context.WithCancel(context.Background()) errc := make(chan error, 1) @@ -608,9 +258,7 @@ func TestRun(t *testing.T) { } time.Sleep(20 * time.Millisecond) } - req, _ := http.NewRequest(http.MethodGet, "http://"+cfg.Listen+"/x", nil) - req.Header.Set("Authorization", "Bearer "+r.mint(t, keycloakClaims(time.Now()))) - resp, err := http.DefaultClient.Do(req) + resp, err := http.Get("http://" + cfg.Listen + "/x") if err != nil { t.Fatal(err) } @@ -618,6 +266,16 @@ func TestRun(t *testing.T) { if resp.StatusCode != 200 { t.Fatalf("proxied status %d", resp.StatusCode) } + resp, err = http.Get("http://" + cfg.HealthListen + "/activity") + if err != nil { + t.Fatal(err) + } + var body activityReport + _ = json.NewDecoder(resp.Body).Decode(&body) + _ = resp.Body.Close() + if body.LastRequest < time.Now().Add(-time.Minute).Unix() { + t.Errorf("/activity %+v", body) + } cancel() if err := <-errc; err != nil { t.Fatalf("Run: %v", err) @@ -625,15 +283,13 @@ func TestRun(t *testing.T) { } func TestRunPortInUse(t *testing.T) { - r := newRealm(t) busy, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) } defer func() { _ = busy.Close() }() - cfg := testConfig(r) - cfg.Listen = freeAddr(t) - cfg.HealthListen = busy.Addr().String() + u, _ := url.Parse("http://127.0.0.1:1") + cfg := Config{Upstream: u, Listen: freeAddr(t), HealthListen: busy.Addr().String()} if err := Run(context.Background(), cfg, quietLog()); err == nil { t.Fatal("Run started with its health port taken") } @@ -650,26 +306,6 @@ func freeAddr(t *testing.T) string { return addr } -func TestFromEnvPassthrough(t *testing.T) { - cfg, err := FromEnv(env(map[string]string{ - "AUTHZ_UPSTREAM": "http://127.0.0.1:8081", - "AUTHZ_PASSTHROUGH": "true", - })) - if err != nil || !cfg.Passthrough { - t.Fatalf("passthrough needs only an upstream: %+v %v", cfg, err) - } - if _, err := FromEnv(env(map[string]string{ - "AUTHZ_UPSTREAM": "http://127.0.0.1:8081", - "AUTHZ_PASSTHROUGH": "true", - "AUTHZ_GROUPS": "analysts", - })); err == nil || !strings.Contains(err.Error(), "cannot be combined") { - t.Errorf("passthrough with AUTHZ_GROUPS: %v", err) - } - if _, err := FromEnv(env(map[string]string{"AUTHZ_PASSTHROUGH": "true"})); err == nil { - t.Error("passthrough without an upstream should fail") - } -} - func TestActivity(t *testing.T) { now := time.Unix(1_000, 0) a := NewActivity(func() time.Time { return now }) @@ -701,82 +337,23 @@ func TestActivity(t *testing.T) { } } -func TestHandlerRecordsOnlyAllowedRequests(t *testing.T) { - r := newRealm(t) +func TestHandlerRecordsActivity(t *testing.T) { app := newApp(t) - cfg := testConfig(r) - cfg.Upstream, _ = url.Parse(app.URL) + u, _ := url.Parse(app.URL) now := time.Unix(1_000, 0) act := NewActivity(func() time.Time { return now }) - h := NewHandler(cfg, NewVerifier(cfg, oidc.NewRemoteKeySet(context.Background(), r.jwksURL())), http.DefaultTransport, quietLog()) + h := NewHandler(Config{Upstream: u}, http.DefaultTransport, quietLog()) h.Activity = act srv := httptest.NewServer(h) t.Cleanup(srv.Close) now = time.Unix(2_000, 0) - resp, err := http.Get(srv.URL + "/denied") - if err != nil { - t.Fatal(err) - } - _ = resp.Body.Close() - if got := act.LastRequest().Unix(); got != 1_000 { - t.Errorf("a refused request counted as activity: %d", got) - } - req, _ := http.NewRequest(http.MethodGet, srv.URL+"/ok", nil) - req.Header.Set("Authorization", "Bearer "+r.mint(t, keycloakClaims(time.Now()))) - resp, err = http.DefaultClient.Do(req) + resp, err := http.Get(srv.URL + "/ok") if err != nil { t.Fatal(err) } _ = resp.Body.Close() if resp.StatusCode != 200 || act.LastRequest().Unix() != 2_000 { - t.Errorf("allowed request: status %d, last %d", resp.StatusCode, act.LastRequest().Unix()) - } -} - -func TestRunPassthrough(t *testing.T) { - app := newApp(t) - u, _ := url.Parse(app.URL) - cfg := Config{Upstream: u, Passthrough: true, Listen: freeAddr(t), HealthListen: freeAddr(t)} - - ctx, cancel := context.WithCancel(context.Background()) - errc := make(chan error, 1) - go func() { errc <- Run(ctx, cfg, quietLog()) }() - - deadline := time.Now().Add(5 * time.Second) - for { - resp, err := http.Get("http://" + cfg.HealthListen + "/readyz") - if err == nil { - _ = resp.Body.Close() - if resp.StatusCode == 200 { - break - } - } - if time.Now().After(deadline) { - t.Fatal("never ready") - } - time.Sleep(20 * time.Millisecond) - } - resp, err := http.Get("http://" + cfg.Listen + "/x") - if err != nil { - t.Fatal(err) - } - _ = resp.Body.Close() - if resp.StatusCode != 200 { - t.Fatalf("passthrough without a token: status %d", resp.StatusCode) - } - resp, err = http.Get("http://" + cfg.HealthListen + "/activity") - if err != nil { - t.Fatal(err) - } - var body activityReport - _ = json.NewDecoder(resp.Body).Decode(&body) - _ = resp.Body.Close() - if body.LastRequest < time.Now().Add(-time.Minute).Unix() { - t.Errorf("/activity %+v", body) - } - cancel() - if err := <-errc; err != nil { - t.Fatalf("Run: %v", err) + t.Errorf("status %d, last %d", resp.StatusCode, act.LastRequest().Unix()) } } diff --git a/authz/internal/gate/jwks.go b/authz/internal/gate/jwks.go deleted file mode 100644 index 2222895..0000000 --- a/authz/internal/gate/jwks.go +++ /dev/null @@ -1,73 +0,0 @@ -package gate - -import ( - "context" - "encoding/json" - "fmt" - "io" - "log/slog" - "net/http" - "sync/atomic" - "time" -) - -// KeyWatch reports readiness: it fetches the JWKS until one fetch returns -// at least one key, so a wrong AUTHZ_JWKS_URL never becomes Ready. Token -// verification keeps its own cache (go-oidc refetches on unknown key IDs). -type KeyWatch struct { - URL string - Client *http.Client - Every time.Duration - Log *slog.Logger - ready atomic.Bool -} - -// Ready reports whether a fetch has succeeded. -func (k *KeyWatch) Ready() bool { return k.ready.Load() } - -// Run fetches until the first success or until ctx ends. -func (k *KeyWatch) Run(ctx context.Context) { - for attempt := 1; ; attempt++ { - err := k.fetch(ctx) - if err == nil { - k.ready.Store(true) - k.Log.Info("realm signing keys fetched", "url", k.URL) - return - } - if attempt == 1 || attempt%30 == 0 { - k.Log.Warn("cannot fetch the realm's signing keys yet", "url", k.URL, "attempt", attempt, "error", err.Error()) - } - select { - case <-ctx.Done(): - return - case <-time.After(k.Every): - } - } -} - -func (k *KeyWatch) fetch(ctx context.Context) error { - ctx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() - req, err := http.NewRequestWithContext(ctx, http.MethodGet, k.URL, nil) - if err != nil { - return err - } - resp, err := k.Client.Do(req) - if err != nil { - return err - } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("status %s", resp.Status) - } - var set struct { - Keys []json.RawMessage `json:"keys"` - } - if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&set); err != nil { - return fmt.Errorf("decoding key set: %w", err) - } - if len(set.Keys) == 0 { - return fmt.Errorf("key set is empty") - } - return nil -} diff --git a/authz/internal/gate/proxy.go b/authz/internal/gate/proxy.go index c292b97..6ee3e50 100644 --- a/authz/internal/gate/proxy.go +++ b/authz/internal/gate/proxy.go @@ -12,22 +12,16 @@ import ( "time" ) -// Handler checks every request's bearer token and proxies the allowed ones -// to the upstream app with the Authorization header intact. In passthrough -// mode (no verifier) it proxies every request unchecked. +// Handler proxies every request to the upstream app unchanged. The gateway +// has already signed the user in and checked access.groups. type Handler struct { - verifier *Verifier // Activity, when set, records each request that reaches the app. Activity *Activity proxy *httputil.ReverseProxy - log *slog.Logger - // forbidden is the 403 body: it names the allowed groups. - forbidden string } -// NewHandler proxies to cfg.Upstream through transport. v may be nil only -// when cfg.Passthrough is set. -func NewHandler(cfg Config, v *Verifier, transport http.RoundTripper, log *slog.Logger) *Handler { +// NewHandler proxies to cfg.Upstream through transport. +func NewHandler(cfg Config, transport http.RoundTripper, log *slog.Logger) *Handler { upstream := cfg.Upstream proxy := &httputil.ReverseProxy{ // Rewrite drops inbound X-Forwarded-* headers; the gateway's are @@ -60,40 +54,11 @@ func NewHandler(cfg Config, v *Verifier, transport http.RoundTripper, log *slog. http.Error(w, "Bad Gateway: the app is not responding", http.StatusBadGateway) }, } - forbidden := "Forbidden: this app is restricted to groups: " + strings.Join(cfg.Groups, ", ") - return &Handler{verifier: v, proxy: proxy, log: log, forbidden: forbidden} + return &Handler{proxy: proxy} } +// ServeHTTP proxies the request, recording it as activity. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if h.verifier == nil { - h.forward(w, r) - return - } - authz := r.Header.Get("Authorization") - if authz == "" { - // Distinct from a denial: the gateway forwarded no token at all, - // which means forwardAccessToken is off or the request bypassed it. - h.log.Info("denied", "status", http.StatusUnauthorized, "reason", "no Authorization header", "path", r.URL.Path) - w.Header().Set("WWW-Authenticate", `Bearer realm="app"`) - http.Error(w, "Unauthorized: no access token was forwarded to this app", http.StatusUnauthorized) - return - } - scheme, token, ok := strings.Cut(authz, " ") - token = strings.TrimSpace(token) - if !ok || !strings.EqualFold(scheme, "Bearer") || token == "" { - h.forbid(w, r, "Authorization header is not a bearer token") - return - } - if err := h.verifier.Verify(r.Context(), token); err != nil { - h.forbid(w, r, err.Error()) - return - } - h.forward(w, r) -} - -// forward proxies an allowed request, recording it as activity: denied -// requests never keep an idle App awake. -func (h *Handler) forward(w http.ResponseWriter, r *http.Request) { if h.Activity != nil { h.Activity.begin() defer h.Activity.end() @@ -101,18 +66,12 @@ func (h *Handler) forward(w http.ResponseWriter, r *http.Request) { h.proxy.ServeHTTP(w, r) } -func (h *Handler) forbid(w http.ResponseWriter, r *http.Request, reason string) { - h.log.Info("denied", "status", http.StatusForbidden, "reason", reason, "path", r.URL.Path) - http.Error(w, h.forbidden, http.StatusForbidden) -} - -// Health serves the token-free probe endpoints: /healthz (the process is -// up), /readyz (the realm's key set has been fetched at least once), +// Health serves the probe endpoints: /healthz and /readyz (the process is +// up), // /upstreamz (the app container accepts connections; the app container's // own probes use it, so the app can listen on loopback only) and /activity // (when the app last served a request, for the operator's idle check). type Health struct { - Ready func() bool Upstream string // host:port Dial func(ctx context.Context, network, addr string) (net.Conn, error) Activity *Activity @@ -120,13 +79,7 @@ type Health struct { func (h *Health) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { - case "/healthz": - _, _ = fmt.Fprintln(w, "ok") - case "/readyz": - if !h.Ready() { - http.Error(w, "waiting for the realm's signing keys", http.StatusServiceUnavailable) - return - } + case "/healthz", "/readyz": _, _ = fmt.Fprintln(w, "ok") case "/upstreamz": ctx, cancel := context.WithTimeout(r.Context(), 2*time.Second) diff --git a/authz/internal/gate/run.go b/authz/internal/gate/run.go index 2ec04db..20c178d 100644 --- a/authz/internal/gate/run.go +++ b/authz/internal/gate/run.go @@ -7,8 +7,6 @@ import ( "net" "net/http" "time" - - "github.com/coreos/go-oidc/v3/oidc" ) // Run serves the proxy and the health endpoints until ctx ends, then shuts @@ -28,19 +26,7 @@ func Run(ctx context.Context, cfg Config, log *slog.Logger) error { ExpectContinueTimeout: time.Second, } - // Passthrough (an unrestricted App the operator may idle) checks no - // tokens: it only records activity, so it is ready at once. - var verifier *Verifier - ready := func() bool { return true } - if !cfg.Passthrough { - jwksClient := &http.Client{Timeout: 10 * time.Second} - keys := oidc.NewRemoteKeySet(oidc.ClientContext(ctx, jwksClient), cfg.JWKSURL) - verifier = NewVerifier(cfg, keys) - watch := &KeyWatch{URL: cfg.JWKSURL, Client: jwksClient, Every: 2 * time.Second, Log: log} - go watch.Run(ctx) - ready = watch.Ready - } - handler := NewHandler(cfg, verifier, upstream, log) + handler := NewHandler(cfg, upstream, log) handler.Activity = activity // No ReadTimeout/WriteTimeout: they would cut WebSockets and streamed @@ -55,7 +41,7 @@ func Run(ctx context.Context, cfg Config, log *slog.Logger) error { }, { Addr: cfg.HealthListen, - Handler: &Health{Ready: ready, Upstream: cfg.Upstream.Host, Dial: dialer.DialContext, Activity: activity}, + Handler: &Health{Upstream: cfg.Upstream.Host, Dial: dialer.DialContext, Activity: activity}, ReadHeaderTimeout: 5 * time.Second, IdleTimeout: 30 * time.Second, }, diff --git a/authz/internal/gate/verify.go b/authz/internal/gate/verify.go deleted file mode 100644 index 64cf8a3..0000000 --- a/authz/internal/gate/verify.go +++ /dev/null @@ -1,159 +0,0 @@ -package gate - -import ( - "context" - "encoding/json" - "fmt" - "slices" - "strings" - "time" - - "github.com/coreos/go-oidc/v3/oidc" -) - -// leeway absorbs clock skew between Keycloak and this node for exp and nbf. -const leeway = 30 * time.Second - -// signingAlgs are the asymmetric algorithms a token may be signed with; -// HMAC and "none" are never accepted. Keycloak signs with RS256 by default. -var signingAlgs = []string{ - oidc.RS256, oidc.RS384, oidc.RS512, - oidc.PS256, oidc.PS384, oidc.PS512, - oidc.ES256, oidc.ES384, oidc.ES512, -} - -// Denial is why a token was refused. Reason is safe to log: it never -// contains the token or claim values. -type Denial struct{ Reason string } - -func (d *Denial) Error() string { return d.Reason } - -func deny(format string, a ...any) error { return &Denial{Reason: fmt.Sprintf(format, a...)} } - -// Verifier checks a bearer access token: signature against the realm's -// JWKS, exp/nbf, iss, the client binding, then the allow lists. -type Verifier struct { - jwt *oidc.IDTokenVerifier - issuers []string - clientID string - groups map[string]bool - groupsClaim string - now func() time.Time -} - -// NewVerifier verifies with keys (an *oidc.RemoteKeySet in production). -func NewVerifier(cfg Config, keys oidc.KeySet) *Verifier { - v := &Verifier{ - issuers: cfg.Issuers, - clientID: cfg.ClientID, - groups: map[string]bool{}, - groupsClaim: cfg.GroupsClaim, - now: time.Now, - } - for _, g := range cfg.Groups { - v.groups[normalizeGroup(g)] = true - } - // go-oidc checks the signature and algorithm only; the claim checks - // below accept several issuers, use azp and allow clock skew both ways. - v.jwt = oidc.NewVerifier("", keys, &oidc.Config{ - SkipClientIDCheck: true, - SkipIssuerCheck: true, - SkipExpiryCheck: true, - SupportedSigningAlgs: signingAlgs, - }) - return v -} - -// Verify returns nil when the token may pass and a *Denial otherwise. A key -// set that cannot be fetched also denies (fail closed). -func (v *Verifier) Verify(ctx context.Context, raw string) error { - tok, err := v.jwt.Verify(ctx, raw) - if err != nil { - return deny("invalid token: signature, algorithm or format") - } - var c map[string]json.RawMessage - if err := tok.Claims(&c); err != nil { - return deny("invalid token: claims are not a JSON object") - } - - now := v.now() - var exp, nbf *float64 - if err := optional(c, "exp", &exp); err != nil || exp == nil { - return deny("invalid token: no exp") - } - if now.After(unix(*exp).Add(leeway)) { - return deny("token expired") - } - if err := optional(c, "nbf", &nbf); err != nil { - return deny("invalid token: nbf") - } - if nbf != nil && now.Add(leeway).Before(unix(*nbf)) { - return deny("token not valid yet (nbf)") - } - - var typ string - if err := optional(c, "typ", &typ); err != nil { - return deny("invalid token: typ") - } - // Keycloak marks access tokens "Bearer"; an ID or refresh token for the - // same client is not a credential for the app. - if typ != "" && !strings.EqualFold(typ, "Bearer") { - return deny("not an access token (typ %q)", typ) - } - - if !slices.Contains(v.issuers, tok.Issuer) { - return deny("issuer not accepted") - } - - var azp string - if err := optional(c, "azp", &azp); err != nil { - return deny("invalid token: azp") - } - if azp != v.clientID && !slices.Contains(tok.Audience, v.clientID) { - return deny("token was issued to another client") - } - - if v.groupsAllow(c) { - return nil - } - return deny("user is not in an allowed group") -} - -// groupsAllow reports whether the groups claim (a string array, or a single -// string) names an allowed group. A leading "/" (Keycloak's full group -// path) is ignored on both sides. -func (v *Verifier) groupsAllow(c map[string]json.RawMessage) bool { - raw, ok := c[v.groupsClaim] - if !ok || len(v.groups) == 0 { - return false - } - var list []string - if err := json.Unmarshal(raw, &list); err != nil { - var one string - if err := json.Unmarshal(raw, &one); err != nil { - return false - } - list = []string{one} - } - for _, g := range list { - if v.groups[normalizeGroup(g)] { - return true - } - } - return false -} - -func normalizeGroup(g string) string { return strings.TrimPrefix(strings.TrimSpace(g), "/") } - -// optional decodes claim name into dst when it is present. -func optional(c map[string]json.RawMessage, name string, dst any) error { - raw, ok := c[name] - if !ok { - return nil - } - return json.Unmarshal(raw, dst) -} - -func unix(sec float64) time.Time { - return time.Unix(0, int64(sec*float64(time.Second))) -} diff --git a/authz/main.go b/authz/main.go index eaa1ea6..6ce5b25 100644 --- a/authz/main.go +++ b/authz/main.go @@ -1,25 +1,18 @@ -// Command apps-authz is the sidecar the apps-operator puts in front of an -// App's container when the App restricts who may open it. It is configured -// only through environment variables: +// Command apps-authz is the sidecar the apps-operator puts in front of the +// app container of Apps that can idle and of image Apps. It proxies every +// request to the app and reports, on the health port's /activity, when the +// app last served one, which the operator reads to scale idle Apps to zero. +// It checks no tokens: sign-in and access.groups are enforced at the gateway +// (nebari-operator v0.2.0+). It is configured only through environment +// variables: // // AUTHZ_LISTEN proxy address (default :8080) -// AUTHZ_HEALTH_LISTEN /healthz, /readyz, /upstreamz, no token (default :8082) +// AUTHZ_HEALTH_LISTEN /healthz, /readyz, /upstreamz, /activity (default :8082) // AUTHZ_UPSTREAM the app container, e.g. http://127.0.0.1:8081 -// AUTHZ_ISSUERS accepted iss values, comma-separated (public and in-cluster realm URL) -// AUTHZ_JWKS_URL the realm's certs endpoint -// AUTHZ_CLIENT_ID the gateway's OIDC client; must equal azp or appear in aud -// AUTHZ_GROUPS allowed groups, comma-separated (required) -// AUTHZ_GROUPS_CLAIM claim holding the user's groups (default groups) -// AUTHZ_PASSTHROUGH true: check no tokens, only record activity (unrestricted Apps that may idle) // -// A request passes when its bearer access token verifies (signature against -// the realm's JWKS, exp/nbf, iss, client) and the user is in any allowed -// group. AUTHZ_GROUPS is required unless AUTHZ_PASSTHROUGH is set. A request without an Authorization header gets 401 (the -// gateway forwarded no token); every other refusal is 403. -// -// In passthrough mode every request is proxied unchecked. Either way the -// health port's /activity reports when the app last served a request, which -// the operator reads to scale idle Apps to zero. +// It refuses to start when AUTHZ_GROUPS (or another setting of the enforcing +// mode it no longer has) is set, so an App an older operator meant to restrict +// never runs open. package main import ( @@ -42,8 +35,7 @@ func main() { } ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() - log.Info("starting", "upstream", cfg.Upstream.String(), "issuers", cfg.Issuers, - "clientID", cfg.ClientID, "groups", len(cfg.Groups), "passthrough", cfg.Passthrough) + log.Info("starting", "upstream", cfg.Upstream.String()) if err := gate.Run(ctx, cfg, log); err != nil { log.Error("server failed", "error", err.Error()) os.Exit(1) diff --git a/charts/nebari-apps/README.md b/charts/nebari-apps/README.md index 147a6e9..b27bddb 100644 --- a/charts/nebari-apps/README.md +++ b/charts/nebari-apps/README.md @@ -49,14 +49,14 @@ wrong type fails the install. | authz.image.pullPolicy | string | `"IfNotPresent"` | Image pull policy. | | authz.image.repository | string | `"ghcr.io/nebari-dev/apps-pack/apps-authz"` | apps-authz image repository. | | authz.image.tag | string | `""` | Image tag. Empty defaults to the chart's appVersion. | -| authz.resources | object | `{"limits":{"cpu":"100m","memory":"64Mi"},"requests":{"cpu":"10m","memory":"32Mi"}}` | Sidecar resources, per restricted app pod (on top of the app container's own). | +| authz.resources | object | `{"limits":{"cpu":"100m","memory":"64Mi"},"requests":{"cpu":"10m","memory":"32Mi"}}` | Sidecar resources, per app pod that runs it (on top of the app container's own). | | clusterDomain | string | `""` | Required. The cluster's base domain; there is no sensible default. | | fullnameOverride | string | `""` | Override the full resource-name prefix. | | gateway | string | `"public"` | Shared Gateway that app NebariApps attach to: `public` or `internal`. | | gitImage | string | `"alpine/git:v2.47.2"` | Image used by init containers to fetch git sources. | -| keycloak.internalUrl | string | `""` | Optional in-cluster Keycloak URL (split horizon) the API, the MCP server and the authz sidecar fetch JWKS from. The sidecar accepts tokens issued under either URL. | +| keycloak.internalUrl | string | `""` | Optional in-cluster Keycloak URL (split horizon) the API and the MCP server fetch JWKS from. | | keycloak.realm | string | `"nebari"` | Keycloak realm. | -| keycloak.url | string | `""` | Browser-facing Keycloak base URL, e.g. `https://keycloak.example.ai/auth`. Required when `api.auth.enabled`, and (or `internalUrl`) for apps that list `access.groups`. | +| keycloak.url | string | `""` | Browser-facing Keycloak base URL, e.g. `https://keycloak.example.ai/auth`. Required when `api.auth.enabled`. | | mcp.enabled | bool | `true` | Deploy the MCP server. Agents connect at `https:///mcp` (streamable HTTP); the UI's nginx proxies `/mcp` to it, so there is no separate hostname. When off, `/mcp` returns 404. | | mcp.extraEnv | list | `[]` | Extra environment variables for apps-mcp, after the chart's own (an entry with the same name wins). | | mcp.image.pullPolicy | string | `"IfNotPresent"` | Image pull policy. | diff --git a/charts/nebari-apps/crds/apps.nebari.dev_apps.yaml b/charts/nebari-apps/crds/apps.nebari.dev_apps.yaml index bb4e11b..1e59e02 100644 --- a/charts/nebari-apps/crds/apps.nebari.dev_apps.yaml +++ b/charts/nebari-apps/crds/apps.nebari.dev_apps.yaml @@ -62,13 +62,17 @@ spec: properties: groups: description: |- - Groups are the Keycloak/OIDC groups allowed to open a private app. - Enforced in the app pod by the authz sidecar, which checks the access - token the gateway forwards: everyone else gets 403. A user in any - listed group is let in. Empty allows any signed-in realm user. Also - forwarded to the NebariApp, where it sets landing-page tile visibility. - Changes apply when the user's access token - refreshes (Keycloak's access token lifespan, 5 minutes by default). + Groups are the Keycloak groups allowed to open a private app. Enforced + at the gateway (NebariApp auth.groups): a signed-in user in none of + them gets 403 before the request reaches the app. A user in any listed + group is let in. Empty allows any signed-in realm user. Entries are + full group paths matched exactly: "finance" is "/finance", and + "/finance" does not admit members of "/finance/analysts" only, so list + each subgroup that needs access. A bare top-level name is created in + Keycloak if missing; a path must already exist. The groups also set + landing-page tile visibility. Changes apply when the user's access + token refreshes (Keycloak's access token lifespan, 5 minutes by + default). items: type: string type: array @@ -81,8 +85,9 @@ spec: properties: groups: description: |- - Groups are Keycloak/OIDC groups whose members may manage the app - (a leading "/" is ignored, as for api.adminGroups). + Groups are Keycloak groups whose members may manage the app, matched + as full group paths like access.groups ("finance" is "/finance", which + does not include "/finance/analysts"). items: type: string type: array @@ -330,8 +335,8 @@ spec: serving its source as static content: the operator installs the pixi environment from the app source and executes `pixi run `. Not allowed with source type image, which runs its own server. - The task must start a server on $PORT: 8080, or 8081 when access.groups - restricts the app or it can idle (the authz sidecar takes 8080). + The task must start a server on $PORT: 8080, or 8081 when the app can + idle (the authz sidecar takes 8080). maxLength: 64 type: string replicas: diff --git a/charts/nebari-apps/templates/operator-deployment.yaml b/charts/nebari-apps/templates/operator-deployment.yaml index 14151c9..5cda46f 100644 --- a/charts/nebari-apps/templates/operator-deployment.yaml +++ b/charts/nebari-apps/templates/operator-deployment.yaml @@ -35,13 +35,6 @@ spec: - --authz-image={{ include "nebari-apps.authzImage" . }} - --authz-image-pull-policy={{ .Values.authz.image.pullPolicy }} - {{ printf "--authz-resources=%s" (toJson .Values.authz.resources) | quote }} - - --keycloak-realm={{ .Values.keycloak.realm }} - {{- with .Values.keycloak.url }} - - --keycloak-url={{ . }} - {{- end }} - {{- with .Values.keycloak.internalUrl }} - - --keycloak-internal-url={{ . }} - {{- end }} - {{ printf "--default-static-resources=%s" (toJson .Values.apps.defaultResources.static) | quote }} - {{ printf "--default-python-resources=%s" (toJson .Values.apps.defaultResources.python) | quote }} {{- if not .Values.tls.enabled }} diff --git a/charts/nebari-apps/values.schema.json b/charts/nebari-apps/values.schema.json index a7d0c17..53af824 100644 --- a/charts/nebari-apps/values.schema.json +++ b/charts/nebari-apps/values.schema.json @@ -1090,7 +1090,7 @@ }, "authz": { "type": "object", - "description": "The apps-authz sidecar that enforces access.groups in app pods.", + "description": "The apps-authz sidecar in front of app pods that can idle or run an image (activity tracking and the front port; access.groups are enforced at the gateway).", "properties": { "image": { "type": "object", diff --git a/charts/nebari-apps/values.yaml b/charts/nebari-apps/values.yaml index 2d84404..9d8332c 100644 --- a/charts/nebari-apps/values.yaml +++ b/charts/nebari-apps/values.yaml @@ -327,14 +327,12 @@ mcp: # ============================================================================= keycloak: # -- Browser-facing Keycloak base URL, e.g. - # `https://keycloak.example.ai/auth`. Required when `api.auth.enabled`, - # and (or `internalUrl`) for apps that list `access.groups`. + # `https://keycloak.example.ai/auth`. Required when `api.auth.enabled`. url: "" # -- Keycloak realm. realm: nebari - # -- Optional in-cluster Keycloak URL (split horizon) the API, the MCP - # server and the authz sidecar fetch JWKS from. The sidecar accepts tokens - # issued under either URL. + # -- Optional in-cluster Keycloak URL (split horizon) the API and the MCP + # server fetch JWKS from. internalUrl: "" # ============================================================================= @@ -388,10 +386,10 @@ gitImage: alpine/git:v2.47.2 # `pixi` binary and run as a non-root user. pythonImage: ghcr.io/prefix-dev/pixi:0.68.1-noble -# The apps-authz sidecar. Apps that list `access.groups` -# get it in front of the app container: it checks the access token the -# gateway forwards against the Keycloak realm (`keycloak.url`, and -# `keycloak.internalUrl` when set) and returns 403 to everyone else. +# The apps-authz sidecar. Apps that can idle and image Apps get it in front of +# the app container: it proxies every request and records activity for the +# idle check. It checks no tokens; `access.groups` are enforced at the gateway +# by nebari-operator. authz: image: # -- apps-authz image repository. @@ -400,7 +398,7 @@ authz: tag: "" # -- Image pull policy. pullPolicy: IfNotPresent - # -- Sidecar resources, per restricted app pod (on top of the app + # -- Sidecar resources, per app pod that runs it (on top of the app # container's own). resources: requests: { cpu: 10m, memory: 32Mi } diff --git a/dev/Makefile b/dev/Makefile index 72deaea..c525319 100644 --- a/dev/Makefile +++ b/dev/Makefile @@ -52,8 +52,8 @@ API_IMG := apps-api:dev UI_IMG := apps-ui:dev MCP_IMG := apps-mcp:dev AUTHZ_IMG := apps-authz:dev -# In-cluster Keycloak from the nebari-operator dev scripts: the authz sidecar -# of apps that list access.groups fetches the realm's keys from it. +# In-cluster Keycloak from the nebari-operator dev scripts: apps-api and +# apps-mcp fetch the realm's keys from it when auth is on. KEYCLOAK_INTERNAL_URL := http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080 APP_NAMESPACE := apps @@ -121,7 +121,14 @@ _cluster-create: _clone-operator _kind-create _services _gateway-nodeport _keycl @echo "" @echo "Run 'make up' to build the operator and deploy an example App." +# A clone of an older pin (from before .nebari-operator-version changed) is +# replaced, so the dev scripts match the nebari-operator release installed. _clone-operator: + @if [ -d "$(OPERATOR_REPO)" ] && \ + [ "$$(git -C $(OPERATOR_REPO) describe --tags --exact-match 2>/dev/null)" != "$(OPERATOR_REF)" ]; then \ + echo "Cached nebari-operator is not $(OPERATOR_REF); re-cloning..."; \ + rm -rf $(OPERATOR_REPO); \ + fi @if [ ! -d "$(OPERATOR_REPO)" ]; then \ echo "Cloning nebari-operator ($(OPERATOR_REF)) into $(OPERATOR_REPO)..."; \ mkdir -p $(CACHE_DIR); \ diff --git a/docs/PLAN.md b/docs/PLAN.md index ab69614..53f1450 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -45,7 +45,7 @@ Companion to [`DESIGN.md`](./DESIGN.md). Phased, each phase ends with a usable i > (mirroring software-pack-template, the adopted source of truth) rather than k3d/Tilt; > app URLs are `.apps.` (dedicated apps zone); TLS is chart-toggleable > (`tls.enabled`, off in local dev). NebariApp contract pinned to nebari-operator -> `v0.1.0-alpha.20` with a contract test. +> `v0.2.0` with a contract test. ## Phase 1 — apps-api + CRUD ✅ Complete **Goal:** create/manage apps over HTTP; CR is written by the API, not by hand. @@ -163,7 +163,7 @@ Companion to [`DESIGN.md`](./DESIGN.md). Phased, each phase ends with a usable i ## Risks - **nebari-operator `NebariApp` contract drift** — pin the version; add a contract test. - ✅ *Mitigated: pinned to `v0.1.0-alpha.20`, contract tests in the operator suite.* + ✅ *Mitigated: pinned to `v0.2.0`, contract tests in the operator suite.* - **Untrusted agent-generated content** — tenancy + hardening must land before any public exposure. ⏳ *Pod hardening + namespace tenancy shipped; NetworkPolicies remain (Phase 6).* - **Keycloak device-flow client provisioning** — confirm nebari-operator exposes this for the MCP. diff --git a/docs/src/content/docs/api-reference.md b/docs/src/content/docs/api-reference.md index aaa9007..85f824c 100644 --- a/docs/src/content/docs/api-reference.md +++ b/docs/src/content/docs/api-reference.md @@ -107,8 +107,7 @@ create. To launch a **Python/pixi app**, add `"pixiTask": ""` to `runtime` — the source (any type) must then contain a `pixi.toml` or `pyproject.toml` at its root, and the task -must start a server on `0.0.0.0:$PORT` (8080, or 8081 when `access.groups` is set -or the app can idle). See [Launching apps](/launching-apps/#python-apps-pixi). +must start a server on `0.0.0.0:$PORT` (8080, or 8081 when the app can idle). See [Launching apps](/launching-apps/#python-apps-pixi). To launch a **container image**, send an `image` source: @@ -194,7 +193,7 @@ curl -X POST https://apps.example.ai/api/v1/apps/upload \ | `404` | App (or its pods, for logs) not found. | | `409` | An app with that name already exists in the namespace, or the subdomain is already used by another app. | | `413` | Request body over 2 MiB, secret values over 256 KiB, or inline files over the ~900 KB inline cap (`MAX_UPLOAD_BYTES`), whether they come from an upload (or its extracted contents) or from `source.inline.files` in a JSON `POST`/`PATCH`. Zip members are checked against the cap before they are decompressed. | -| `422` | Invalid request: unsupported source type, missing required fields, an `access.groups` entry that is blank or contains a comma, an `access` field the API does not know (such as the removed `users`), an invalid, duplicated or env-clashing secret name, or a `runtime.env` reference to a Secret or to another app's ConfigMap. | +| `422` | Invalid request: unsupported source type, missing required fields, an `access.groups` or `access.managers.groups` entry that names no group (blank, or only `/`), an `access` field the API does not know (such as the removed `users`), an invalid, duplicated or env-clashing secret name, or a `runtime.env` reference to a Secret or to another app's ConfigMap. | | `502` | The app's Secret could not be written (the reason is in the API's log, never a value). | Error bodies are `{"detail": "..."}` with an actionable message. diff --git a/docs/src/content/docs/app-crd-reference.md b/docs/src/content/docs/app-crd-reference.md index 8866e98..341403c 100644 --- a/docs/src/content/docs/app-crd-reference.md +++ b/docs/src/content/docs/app-crd-reference.md @@ -172,14 +172,14 @@ operator and apps-api refuse to start with a malformed one. | `replicas` | int | No | `1` | Desired replicas, `0`–`10`. `0` stops the app (phase `Stopped`). | | `keepAlive` | bool | No | `false` | `true` exempts the app from [scale to zero](#scale-to-zero): it keeps running however long it goes without a request, whatever `idleTimeoutMinutes` or the cluster default say. | | `idleTimeoutMinutes` | int | No | chart `apps.idle.timeoutMinutes` | Minutes without a request before the operator scales the app to zero (phase `Idle`), `5`–`43200`. Unset follows the cluster default (`0` there keeps apps running), so an App can opt in on a cluster that does not idle by default. Ignored when `keepAlive` is `true` or `replicas` is `0`. See [Scale to zero](#scale-to-zero). | -| `pixiTask` | string | No | — | Not allowed with `source.type: image`. When set, the app runs as a **Python/pixi service**: the operator copies the source into a writable workspace, runs `pixi install` (`--locked` when a `pixi.lock` is present), then `pixi run `. The task must start a server on `0.0.0.0:$PORT` (`PORT` and `HOME=/app` are injected; `PORT` is 8080, or 8081 when the sidecar takes 8080: `access.groups` is set, or the app can idle). Max 64 chars. | +| `pixiTask` | string | No | — | Not allowed with `source.type: image`. When set, the app runs as a **Python/pixi service**: the operator copies the source into a writable workspace, runs `pixi install` (`--locked` when a `pixi.lock` is present), then `pixi run `. The task must start a server on `0.0.0.0:$PORT` (`PORT` and `HOME=/app` are injected; `PORT` is 8080, or 8081 when the sidecar takes 8080 because the app can idle). Max 64 chars. | ## spec.access | Field | Type | Required | Description | |---|---|---|---| | `public` | bool | No | `true` disables authentication entirely (anonymous access). | -| `groups` | []string | No | Keycloak groups allowed to open a private app. **Enforced in the app pod** by the authz sidecar (everyone else gets `403`); also passed to the `NebariApp` as `auth.groups`, which hides the landing-page tile from non-members. A user in **any** group gets in; with none listed, any signed-in realm user does. Access is by group only: to let one person in, add them to a group. Changes apply when the user's access token refreshes (Keycloak's lifespan, 5 minutes by default). See [Access control](/launching-apps/#access-control). | +| `groups` | []string | No | Keycloak groups allowed to open a private app. **Enforced at the gateway**: passed to the `NebariApp` as `auth.groups` with `enforceAtGateway: true`, so every other signed-in user gets `403` there; it also hides the landing-page tile from non-members. Entries are **full group paths** matched exactly: `finance` is `/finance`, and `/finance` does not admit members of `/finance/analysts` only, so list each subgroup. A bare top-level name is created in Keycloak if missing; a path must exist. An entry that names no group (`/`) is refused. A user in **any** group gets in; with none listed, any signed-in realm user does. Access is by group only: to let one person in, add them to a group. Changes apply when the user's access token refreshes (Keycloak's lifespan, 5 minutes by default). See [Access control](/launching-apps/#access-control). | | `managers.users` | []string | No | Keycloak usernames (`preferred_username`, case-insensitive) who may **manage** the app through apps-api, the UI and the MCP server: everything its owner can do (edit, stop, start, restart, delete, read status, logs, events and metrics) except change `managers`. Only the owner and `api.adminGroups` members may change the list. Does **not** let them open a restricted app: that is still `public` and `groups`. Ignored by the operator. See [Managers](/launching-apps/#managers). | | `managers.groups` | []string | No | Keycloak groups whose members may manage the app, like `managers.users`. Membership is read from the caller's verified access token (a leading `/` is ignored), the same way `api.adminGroups` is checked, so a group change applies when their token refreshes. | | `subdomain` | string | Yes | Lowercase DNS label, at most 63 characters. The app is served at `https://.`. **Unique across all Apps** in the cluster: the API returns `409` for a taken subdomain, and if two Apps claim one anyway (e.g. via `kubectl`), the oldest keeps it and the other goes `Failed` with `Validated=False`, reason `SubdomainTaken`, and no `NebariApp`. **Immutable** after creation. The operator mirrors it into the `apps.nebari.dev/subdomain` label. | @@ -220,7 +220,7 @@ For each `App` the operator creates (and owns, via `ownerReferences`): |---|---|---| | ConfigMap | `app--content` | Inline source files (inline apps only). | | ConfigMap | `app--server` | The nginx server config with security headers (static apps only). | -| Deployment | `app-` | The app workload (hardened: non-root, no privilege escalation, seccomp `RuntimeDefault`). The `app` container comes first; an App that lists `access.groups`, that can idle, or that runs an image also gets the `authz` sidecar on 8080, with the app on 8081 (an image app on its own `source.image.port`). For an unrestricted App the sidecar runs in passthrough mode and only records activity. Scaled to zero while the App is `Idle`. | +| Deployment | `app-` | The app workload (hardened: non-root, no privilege escalation, seccomp `RuntimeDefault`). The `app` container comes first; an App that can idle or that runs an image also gets the `authz` sidecar on 8080, with the app on 8081 (an image app on its own `source.image.port`). The sidecar proxies every request and records activity; it checks no tokens (`access.groups` are enforced at the gateway). Scaled to zero while the App is `Idle`. | | Service | `app-` | ClusterIP on port 8080 (the app, or the authz sidecar in front of it). | | NebariApp | `app-` | Routing + TLS + auth + (opt-in) landing-page tile, reconciled by the nebari-operator (contract pinned to `%NEBARI_OPERATOR_VERSION%`). While the App is `Idle` or waking, `spec.service` points at the activator Service in the pack's namespace. | @@ -260,8 +260,9 @@ otherwise let anyone who can write an App read any Secret in its namespace throu An App **can idle** when its idle timeout (`runtime.idleTimeoutMinutes`, else the chart's `apps.idle.timeoutMinutes`) is above zero, `runtime.keepAlive` is not `true` and `runtime.replicas` is not `0`. Such an App runs the `authz` sidecar, which records when the app -last served a request; a request still open (a WebSocket, a stream) counts until it ends, and -requests the sidecar refuses (`401`/`403`) do not count. The operator asks every ready pod's +last served a request; a request still open (a WebSocket, a stream) counts until it ends. +Requests the gateway refuses (sign-in, or `403` outside a restricted App's groups) never reach +the pod, so they do not count. The operator asks every ready pod's sidecar every few minutes (a sixth of the timeout, between 30 seconds and 5 minutes) and keeps the answer in `status.lastActivityTime`. @@ -269,8 +270,8 @@ the answer in `status.lastActivityTime`. to zero and the phase becomes `Idle`. The App's spec, including `replicas`, is unchanged. 2. Its `NebariApp` now routes to the **activator**: a page served by the operator itself (behind the `-activator` Service) that says the app is starting, reloads itself every few - seconds, and answers `503` with `Retry-After` for non-browser clients. Sign-in at the gateway - still applies to private apps. + seconds, and answers `503` with `Retry-After` for non-browser clients. Sign-in and + `access.groups` at the gateway still apply, so only someone allowed in can wake the app. 3. The request stamps the `apps.nebari.dev/wake-requested-at` annotation on the App, which counts as a request: the operator scales the Deployment back to `replicas` (phase `Deploying`, message "waking up"). Starting the App from the UI, the API or the `start_app` tool sets the diff --git a/docs/src/content/docs/architecture.md b/docs/src/content/docs/architecture.md index 61b6bc4..3ed535b 100644 --- a/docs/src/content/docs/architecture.md +++ b/docs/src/content/docs/architecture.md @@ -34,8 +34,8 @@ For each `App`, the operator runs an ordered, idempotent pipeline: `RuntimeDefault`): nginx serving the app's content, with git sources cloned by a non-root init container. Static apps run an operator-written nginx config (a `-server` ConfigMap mounted over `/etc/nginx/conf.d`) that adds security headers. - An App restricted to groups also gets the `authz` sidecar (next section), and so - does an App that can idle, where the sidecar records activity. An `image` App runs the + An App that can idle also gets the `authz` sidecar (next section), which records + activity; it checks no tokens. An `image` App runs the given image as the `app` container instead, always behind the sidecar, which keeps 8080 as the one port the Service and NetworkPolicy target while the image listens on its own port. Before the Deployment is applied the operator runs the **idle check** (below) and scales an idle App to zero. @@ -55,8 +55,9 @@ An App that goes its idle timeout without a request is scaled to zero and wakes request ([details](/app-crd-reference/#scale-to-zero)). The pieces: - **Activity.** The `authz` sidecar counts requests that reach the app and serves the time of - the last one on its health port (`GET :8082/activity`). Apps that restrict nobody run it in - passthrough mode, so public and restricted Apps are measured the same way. The operator polls + the last one on its health port (`GET :8082/activity`). It proxies every request: sign-in and + `access.groups` were already enforced at the gateway, so a request that reaches the pod is one + the App lets in, and public and restricted Apps are measured the same way. The operator polls every ready pod (requeueing the App on a timer) and keeps the result in `status.lastActivityTime`. A pod it cannot read counts as active. - **Idle.** Past the timeout the Deployment goes to zero replicas and the phase to `Idle`. @@ -113,15 +114,29 @@ Apps never contain auth code. A private app's `NebariApp` creates an Envoy Gatew `access.public: true` skips the policy entirely. This keeps user workloads honest — there is no token to mishandle inside an app pod. -The gateway only signs users in; it has no group rule -([nebari-operator#153](https://github.com/nebari-dev/nebari-operator/issues/153)). So when a -private App lists `access.groups`, the operator enforces them in the pod: -the `NebariApp` sets `forwardAccessToken: true`, and the Deployment gets the **apps-authz** -sidecar on port 8080 in front of the app container (moved to 8081). The sidecar verifies -the forwarded Keycloak access token against the realm's keys and lets in a member of any -listed group; everyone else gets `403`. Access is by group only: Apps have no per-user -allow list. `access.groups` also goes to the `NebariApp` as `auth.groups` (landing-page tile -visibility). See [Access control](/launching-apps/#access-control). +When a private App lists `access.groups`, the operator passes them to the `NebariApp` as +`auth.groups` with `enforceAtGateway: true`, and nebari-operator (`%NEBARI_OPERATOR_VERSION%` or +later) adds a group rule to the same `SecurityPolicy`: the gateway verifies the user's access +token against the realm's keys and lets in a member of any listed group; every other signed-in +user gets `403` before the request reaches the pod. Entries are full Keycloak group paths +matched exactly (`finance` is `/finance`, and `/finance` does not admit members of +`/finance/analysts` only). Access is by group only: Apps have no per-user allow list. The same +groups set landing-page tile visibility. See [Access control](/launching-apps/#access-control). + +The split between the gateway and the pod: + +| Concern | Where | +|---|---| +| Sign-in (Keycloak SSO) | Gateway (`SecurityPolicy`) | +| `access.groups` (`403` for non-members) | Gateway (`SecurityPolicy` group rule, nebari-operator `%NEBARI_OPERATOR_VERSION%`+) | +| Activity for [scale to zero](#scale-to-zero) | apps-authz sidecar, in Apps that can idle | +| One front port (8080) for an `image` App | apps-authz sidecar, in every `image` App | + +The **apps-authz** sidecar checks no tokens and gets none: the gateway does not forward the +access token to app pods. A restricted App that neither idles nor runs an image has no sidecar. +Because the gateway is the only check, keep app pods unreachable except through it: turn on +`apps.networkPolicy.enabled`, which admits traffic to app pods only from the gateway's +namespace (and the operator's idle check). ## Secrets @@ -218,8 +233,8 @@ The zone is configurable via the `appsDomain` value (default `apps.` | Domain apps are served under: `https://.`. | | `gateway` | `public` | Shared Gateway the UI's and every app's `NebariApp` attach to (`public` \| `internal`). | | `tls.enabled` | `true` | TLS for the UI and every launched app. `false` serves plain HTTP (no certificates); local dev only. | -| `keycloak.url` | — (required when `api.auth.enabled`) | Browser-facing Keycloak base URL, e.g. `https://keycloak.example.ai/auth`. The authz sidecar also accepts tokens issued under it; apps that list `access.groups` need it or `keycloak.internalUrl`. | +| `keycloak.url` | — (required when `api.auth.enabled`) | Browser-facing Keycloak base URL, e.g. `https://keycloak.example.ai/auth`. | | `keycloak.realm` | `nebari` | Realm the API validates tokens against and the UI/MCP log in to. | -| `keycloak.internalUrl` | — | In-cluster Keycloak URL (split horizon). The API and MCP server fetch JWKS from it; the authz sidecar fetches keys from it and accepts it as a token issuer. | +| `keycloak.internalUrl` | — | In-cluster Keycloak URL (split horizon). The API and MCP server fetch JWKS from it. | ## Launched apps @@ -35,7 +35,7 @@ only `clusterDomain` and `keycloak.url` need changing. | `staticImage` | `nginxinc/nginx-unprivileged:1.27-alpine` | Serves static app content; must listen on 8080 as non-root. The operator mounts its nginx server config (security headers, `try_files`) over `/etc/nginx/conf.d`, so a replacement image should be nginx-based or ignore that directory. | | `gitImage` | `alpine/git:v2.47.2` | Init-container image that clones `git` sources. | | `pythonImage` | `ghcr.io/prefix-dev/pixi:0.68.1-noble` | Runs Python/pixi apps (`runtime.pixiTask`); must provide `pixi` and run as non-root. | -| `authz.image.repository` / `tag` / `pullPolicy` | `ghcr.io/nebari-dev/apps-pack/apps-authz` / appVersion / `IfNotPresent` | The sidecar the operator adds to apps that list `access.groups`: it takes port 8080, checks the forwarded access token and returns `403` to everyone else (see [Access control](/launching-apps/#access-control)). Apps that can idle and image apps get it too; without groups it runs in passthrough mode and only records when the app last served a request. | +| `authz.image.repository` / `tag` / `pullPolicy` | `ghcr.io/nebari-dev/apps-pack/apps-authz` / appVersion / `IfNotPresent` | The sidecar the operator adds to apps that can idle and to image apps: it takes port 8080, proxies every request to the app and records when the app last served one. It checks no tokens; `access.groups` are enforced at the gateway (see [Access control](/launching-apps/#access-control)). | | `authz.resources` | `10m`/`32Mi` requests, `100m`/`64Mi` limits | The sidecar's requests/limits, on top of the app container's, in every pod that runs it. | ### Image registries @@ -273,12 +273,14 @@ The pack integrates with the platform through two kinds of `NebariApp` | `gateway` | `gateway` | chart | | `auth.enabled` | `!spec.access.public` | App | | `auth.provider` / `provisionClient` / `scopes` | `keycloak` / `true` / `openid profile email groups` | fixed (private apps) | -| `auth.groups` | `spec.access.groups` | App — landing-page tile visibility; the gateway does not enforce it, the authz sidecar in the app pod does (see [Access control](/launching-apps/#access-control)) | -| `auth.forwardAccessToken` | `true` when `spec.access.groups` is set (private apps) | App — the gateway forwards the access token the authz sidecar checks | -| `auth.enforceAtGateway` | `true`, with `forwardAccessToken` | fixed — the `NebariApp` CRD requires it for `forwardAccessToken` (it is also the default) | +| `auth.groups` | `spec.access.groups`, as written (private apps that list groups) | App — the gateway's group rule: a signed-in user in none of them gets `403`. nebari-operator matches entries as full group paths (`finance` is `/finance`). Also sets landing-page tile visibility (see [Access control](/launching-apps/#access-control)) | +| `auth.enforceAtGateway` | `true` when `spec.access.groups` is set (private apps) | fixed — makes nebari-operator enforce `auth.groups` at the gateway (also its default; set explicitly so a restricted App never depends on it) | | `landingPage.enabled` | `spec.landingPage.enabled`, else `apps.landingPage.enabled` | App, then chart | | `landingPage.displayName` / `description` / `icon` / `category` | `spec.displayName` / `spec.description` / `spec.thumbnail` / `Apps` | App (only when the tile is enabled) | Fields the pack never sets: `auth.users` (Apps restrict access by group only), -`auth.enforceAtGateway` on unrestricted apps (defaults to enforced), +`auth.forwardAccessToken` (no part of the pack reads the token in the app pod, so the gateway +keeps it out of untrusted code), `auth.keycloakConfig` (nebari-operator's default groups mapper +emits the full paths the group rule matches), `auth.enforceAtGateway` on unrestricted apps +(defaults to enforced), `landingPage.priority`, `landingPage.iconLight`/`iconDark`, `landingPage.externalUrl`. diff --git a/docs/src/content/docs/getting-started.md b/docs/src/content/docs/getting-started.md index 28c3f8f..29a65fe 100644 --- a/docs/src/content/docs/getting-started.md +++ b/docs/src/content/docs/getting-started.md @@ -23,9 +23,8 @@ title: Getting started - Optional: metrics-server for per-app live CPU/memory usage in the UI (without it the UI still shows limits and restarts) and the LGTM observability pack for logs. -Read the [Known limitations](#known-limitations) before a demo: group and user restrictions -are enforced in the app pod (by a sidecar), not at the gateway, and users manage only their -own apps (admins manage all of them). +Read the [Known limitations](#known-limitations) before a demo: access is by Keycloak group +(full group paths), and users manage only their own apps (admins manage all of them). ## Install @@ -73,7 +72,7 @@ kubectl apply -n argocd -f examples/argocd-application.yaml ``` This deploys four components. A fifth image, **apps-authz**, is not a Deployment: the -operator adds it as a sidecar to Apps that list `access.groups`, that can idle, or that run an image. +operator adds it as a sidecar to Apps that can idle or that run an image. | Component | Where it ends up | |---|---| @@ -169,14 +168,14 @@ it the HTTPRoute, certificate, and OIDC client) are all garbage-collected. The pack is alpha: the happy path works, and these are the gaps to know about before a demo or pilot. -- **`access.groups` is enforced in the app pod, not at the gateway.** The - nebari-operator `SecurityPolicy` only signs users in - ([nebari-operator#153](https://github.com/nebari-dev/nebari-operator/issues/153)), so a - private app that lists groups gets the apps-authz sidecar, which checks the - forwarded access token and returns `403` to everyone else. Group changes apply when the - user's token refreshes (5 minutes by default). A private app with no groups is open to any - signed-in realm user. Access is by group only; there is no per-user allow list. See - [Access control](/launching-apps/#access-control). +- **Access is by Keycloak group, matched as full paths.** A private app that lists + `access.groups` is enforced at the gateway (nebari-operator `%NEBARI_OPERATOR_VERSION%` or + later), which returns `403` to every other signed-in user. `finance` means `/finance`, and a + parent group does not admit members of its subgroups, so list each subgroup. Group changes + apply when the user's token refreshes (5 minutes by default). A private app with no groups + is open to any signed-in realm user, and there is no per-user allow list. The gateway is the + only check, so turn on `apps.networkPolicy.enabled` to keep other pods from reaching apps + directly. See [Access control](/launching-apps/#access-control). - **Authorization is owner, manager or admin.** The API lets a user see and manage the Apps they launched or are listed as a [manager](/launching-apps/#managers) of; members of `api.adminGroups` manage all of them. Ownership cannot be transferred yet, and anyone with @@ -198,8 +197,8 @@ or pilot. when `apps.idle.timeoutMinutes` (or an App's `runtime.idleTimeoutMinutes`) is set. The first request after idling gets a "starting" page until a pod is ready: seconds for a static app, up to minutes for a Python app, which reinstalls its pixi environment. Any request to an idle - App's URL wakes it, including bots on public apps and signed-in users outside a restricted - App's groups. Apps that can idle carry the authz sidecar, and Python apps there listen on + App's URL wakes it, including bots on public apps (the gateway still keeps users outside a + restricted App's groups out). Apps that can idle carry the authz sidecar, and Python apps there listen on 8081. Idle detection counts HTTP requests through the sidecar only, so an app that works in the background with no visitors needs `runtime.keepAlive: true`. See [Scale to zero](/app-crd-reference/#scale-to-zero). diff --git a/docs/src/content/docs/launching-apps.md b/docs/src/content/docs/launching-apps.md index 17112c1..68019ab 100644 --- a/docs/src/content/docs/launching-apps.md +++ b/docs/src/content/docs/launching-apps.md @@ -10,8 +10,8 @@ This pack deploys three kinds of app: - **Static sites** (HTML/CSS/JS) — served by nginx. - **Python apps** — launched by a [pixi](https://pixi.sh) task (`runtime.pixiTask`); the platform runs `pixi install` then `pixi run `, and the task must start a server - on **0.0.0.0:$PORT** (`PORT` is injected: 8080, or 8081 when the app is - [restricted to groups](#access-control) or can [idle](#idle-apps)). + on **0.0.0.0:$PORT** (`PORT` is injected: 8080, or 8081 when the app can + [idle](#idle-apps)). - **Container images** — a published image (Streamlit, Gradio, Shiny, Dash, a Node service) run as is, when the cluster allows its registry. See [Container images](#container-images). @@ -145,8 +145,8 @@ The contract: `pixi install` (with `--locked` when a `pixi.lock` is present) and then `pixi run `. - The task must start a server listening on **0.0.0.0:$PORT**; `PORT` and `HOME=/app` - are injected. `PORT` is 8080, or 8081 when the authz sidecar takes 8080 (the app lists - groups, or it can [idle](#idle-apps)), so read it rather than hard-coding a port. pixi task strings + are injected. `PORT` is 8080, or 8081 when the authz sidecar takes 8080 (the app can + [idle](#idle-apps)), so read it rather than hard-coding a port. pixi task strings support `$PORT` but not `${PORT:-8080}`; run `PORT=8080 pixi run start` locally. Routing, TLS, and SSO are identical to static apps. - First start resolves and downloads the environment, so cold starts take longer — @@ -192,7 +192,7 @@ What to know: every start; a digest pins the image. - **The port.** Set `port` to where the server listens (on `0.0.0.0` or `127.0.0.1`), from 1024 up and not 8080 or 8082: the pack's sidecar always sits in front of an image app on - 8080, enforcing `access.groups` and recording activity for + 8080, proxying requests to it and recording activity for [idling](#idle-apps). The port is also passed as `$PORT`. - **Non-root.** The image must run as a non-root user, with no capabilities and no Kubernetes credentials. An image whose `USER` is root never starts: the app shows @@ -288,31 +288,38 @@ set, rotate and remove secrets. are redirected to Keycloak and must sign in. The app itself never sees or handles auth. - **Private with `access.groups`** — only members of **any** listed Keycloak group get in. - Everyone else who signs in gets **403**. Access is granted by group only: to let a + Everyone else who signs in gets **403** from the gateway. Entries are full group paths: + `finance` means `/finance`, and a subgroup such as `/finance/analysts` has to be listed + itself. Access is granted by group only: to let a particular person in, add them to one of the app's groups in Keycloak. :::note[How groups are enforced] -The gateway's `SecurityPolicy` only signs users in -([nebari-operator#153](https://github.com/nebari-dev/nebari-operator/issues/153)), so the -pack enforces the groups in the app pod. For an App that lists groups, the -operator sets `forwardAccessToken: true` on its `NebariApp`, so the gateway passes the -user's Keycloak access token to the pod as `Authorization: Bearer …`, and adds the -**apps-authz** sidecar on port 8080 in front of the app container (which moves to 8081). -The sidecar verifies the token against the realm's keys (signature, expiry, issuer, and -that it was issued to the app's own client) and checks its `groups` claim. A request with no token gets `401`; every other refusal is -`403`. - +The gateway enforces the groups. The operator passes them to the App's `NebariApp` as +`auth.groups` with `enforceAtGateway: true`, and nebari-operator adds a group rule to the +app's `SecurityPolicy`: the gateway reads the user's Keycloak access token from the session +cookie, verifies it against the realm's keys, and checks that it was issued to the app's own +client and that its `groups` claim holds a listed group. Everyone else gets `403` from the +gateway, before the request reaches the pod. The app never receives the token. + +- **Groups are full paths, matched exactly.** Keycloak groups nest, and the token carries + each group's full path (`/finance`, `/finance/analysts`). An entry is one group: + `finance` and `/finance` are the same top-level group, and `/finance` does **not** admit + someone who is only in `/finance/analysts`. List each subgroup that needs access, such as + `/finance/analysts`. Matching is case-sensitive. +- **A bare name is created, a path is not.** nebari-operator creates a missing top-level + group named by a bare entry (`finance`) in Keycloak. A path (`/ops/oncall`) must already + exist; until it does, the `NebariApp` reports `GroupsNotResolved` and members of that group + are not let in. - **Group changes apply when the user's access token refreshes**: Keycloak's access-token lifespan, 5 minutes by default. Removing someone from a group does not end a session - they already have open until then. -- Needs nebari-operator `%NEBARI_OPERATOR_VERSION%` or later (for `forwardAccessToken`) and the - chart's `keycloak.url` or `keycloak.internalUrl`; without them the App fails validation - instead of running open. -- The groups are also sent to the `NebariApp` as `auth.groups`, so a landing-page tile - is shown only to their members. -- With the chart's `apps.networkPolicy.enabled`, only the gateway can reach the pod, and - only on 8080. Static apps listen on loopback behind the sidecar; a Python task that - binds `0.0.0.0` is reachable on 8081 from inside the cluster unless that policy is on. + they already have open until then; someone just added should sign out and back in. +- Needs nebari-operator `%NEBARI_OPERATOR_VERSION%` or later, the first release that enforces + `auth.groups` at the gateway, with Envoy Gateway v1.2.0 or later. + ([Upgrading](/upgrading/) has what changes on the way there.) +- The groups also set landing-page tile visibility: a tile is shown only to their members. +- The gateway is the only check, so turn on the chart's `apps.networkPolicy.enabled`: then + only the gateway can reach app pods, on 8080, and nothing inside the cluster can go around + it. ::: ## Managers @@ -329,7 +336,8 @@ app shows up in their Apps list and under **Mine**. - **Managing is not opening.** Managers of a restricted app still need to be in its `access.groups` to open it; add them to one of those groups if they should. - Group membership comes from the caller's verified access token, so adding someone to a - manager group applies when their token refreshes. + manager group applies when their token refreshes. Manager groups are full paths, matched + like `access.groups` (`web-team` is `/web-team`, which does not include `/web-team/frontend`). - Ownership itself cannot be transferred yet. ```yaml diff --git a/docs/src/content/docs/runbook.md b/docs/src/content/docs/runbook.md index d3f5aa7..4d38cd9 100644 --- a/docs/src/content/docs/runbook.md +++ b/docs/src/content/docs/runbook.md @@ -81,9 +81,37 @@ kubectl -n nebari-operator-system logs deploy/nebari-operator-controller-manager run with `tls.enabled=false`. - **Auth not ready.** The nebari-operator could not create the Keycloak client. Check that Keycloak is up and that the nebari-operator's admin credentials are valid. +- **`GroupsNotResolved`, `GroupsEmpty` or `GroupsClaimConflict`.** The App's + `access.groups` cannot be enforced as written (a path missing in Keycloak, entries that + name no group, or a groups mapper in the wrong form), so the gateway denies its users until + it is fixed. The App's `RoutingReady` message carries the reason; see + [Troubleshooting](/troubleshooting/). - **Every App at once.** The shared Gateway (`gateway`) or the nebari-operator itself is down; fix that, and the NebariApps recover without touching the Apps. +## Group access + +`access.groups` are enforced at the gateway, by the group rule nebari-operator adds to each +restricted App's SecurityPolicy (`%NEBARI_OPERATOR_VERSION%` or later). The apps-authz sidecar +in app pods checks nothing. + +```bash +kubectl -n get nebariapp app- -o jsonpath='{.spec.auth.groups}{"\n"}' +kubectl -n get securitypolicy app--security -o jsonpath='{.spec.authorization}{"\n"}' +``` + +- **A member gets `403`.** Compare the token's groups (full paths) with the policy's values: + `/finance` does not admit someone only in `/finance/analysts`. A user just added to a group + signs out and back in; one removed keeps access until their token expires (5 minutes by + default). +- **Everyone signed in gets in.** No `authorization` in the policy: nebari-operator is older + than `%NEBARI_OPERATOR_VERSION%`, or the App lists no groups. +- **Users of restricted Apps were all signed out at once.** Expected once after upgrading + nebari-operator: the access-token cookie of a group-enforcing App is renamed + (`nebari-at--`). See [Upgrading](/upgrading/). +- **Keep the gateway the only way in.** With the chart's `apps.networkPolicy.enabled` off, + other pods in the cluster can reach an app's Service without passing the group rule. + ## Idle Apps do not idle or do not wake `Idle` is a normal phase: the App went its idle timeout (`runtime.idleTimeoutMinutes`, else @@ -122,7 +150,8 @@ kubectl -n nebari-apps logs deploy/nebari-apps-operator --since=30m | grep -E 'i no ready pod: troubleshoot it like any App that does not start ([Troubleshooting](/troubleshooting/)). A Python app reinstalls its environment on every cold start, which can take minutes. - **Turn idling off quickly.** Set `apps.idle.timeoutMinutes=0` and upgrade: Apps without their - own `idleTimeoutMinutes` wake and keep running (their pods roll once to drop the sidecar). + own `idleTimeoutMinutes` wake and keep running (their pods roll once to drop the sidecar, + except image Apps, which always run it). For one App, set `runtime.keepAlive: true`. ## The API is down or returns errors diff --git a/docs/src/content/docs/troubleshooting.md b/docs/src/content/docs/troubleshooting.md index dea7c89..1146b9f 100644 --- a/docs/src/content/docs/troubleshooting.md +++ b/docs/src/content/docs/troubleshooting.md @@ -62,16 +62,33 @@ kubectl -n logs deploy/app- -c app # the app itself `pixi.lock` makes the install `--locked` and faster. - **Python app is `Running` but never ready.** The task must listen on `0.0.0.0:$PORT` (the pod gets `PORT=8080`). A server bound to `127.0.0.1` fails the readiness probe. An App - that lists `access.groups` (or can idle) gets `PORT=8081`, because the authz sidecar - takes 8080: a task that hard-codes 8080 then collides with the sidecar. -- **A restricted app returns `403` to someone who should get in.** The sidecar's log says - why (`kubectl logs deploy/app- -c authz`): not in an allowed group, - token issued to another client, issuer not accepted (check `keycloak.url` and - `keycloak.internalUrl`), or expired. Group changes reach the token on its next refresh - (5 minutes by default). A `401` means the gateway forwarded no token: the `NebariApp` has - no `auth.forwardAccessToken` (nebari-operator older than `%NEBARI_OPERATOR_VERSION%`). -- **A restricted app stays `Failed` with "need the realm's URL" or "--authz-image".** Set - the chart's `keycloak.url` (or `keycloak.internalUrl`) and keep `authz.image`. + that can idle gets `PORT=8081`, because the authz sidecar takes 8080: a task that + hard-codes 8080 then collides with the sidecar. +- **A restricted app returns `403` to someone who should get in.** The gateway refused them: + their access token's `groups` claim holds none of the App's `access.groups`. Entries are + full group paths matched exactly and case-sensitively, so `/finance` does not admit someone + who is only in `/finance/analysts`: list the subgroup itself. Group changes reach the token + on its next refresh (5 minutes by default); someone just added should sign out and back in. + The values the gateway enforces are in the SecurityPolicy: + `kubectl -n get securitypolicy app--security -o jsonpath='{.spec.authorization}'`. + nebari-operator's [troubleshooting guide](https://github.com/nebari-dev/nebari-operator/blob/main/docs/troubleshooting.md) + covers the rest. +- **Every signed-in user gets into a restricted app.** nebari-operator is older than + `%NEBARI_OPERATOR_VERSION%`, which is the first release that enforces `auth.groups` at the + gateway: upgrade it ([Upgrading](/upgrading/)). The SecurityPolicy above has no + `authorization` section then. +- **A restricted app stays `Deploying` with `GroupsNotResolved` in `RoutingReady`.** A path in + `access.groups` (such as `/ops/oncall`) does not exist in Keycloak. Paths are never created: + create the group, or list a top-level group by bare name (`oncall`) to have nebari-operator + create it. `GroupsEmpty` means every entry names no group (such as `/`), and + `GroupsClaimConflict` that a mapper on the app's Keycloak client writes the `groups` claim + in another form; the gateway denies everyone until it is fixed. +- **An app pod's `authz` container exits with "AUTHZ_GROUPS is set".** The authz image is + newer than the apps-operator, which still asks the sidecar to check groups. The sidecar + refuses rather than run the App open: upgrade the operator to the same release as the + image (the chart ships them together). +- **An image app stays `Failed` with "--authz-image".** Image apps always run the sidecar: + keep the chart's `authz.image`. - **`CreateContainerConfigError` with `secret "app--secrets" not found` or `couldn't find key`.** The App lists `runtime.secretKeys` that its Secret does not hold. Set the missing values (UI **Secrets**, `PATCH /apps/{ns}/{name}/secrets`), or, for an App diff --git a/docs/src/content/docs/upgrading.md b/docs/src/content/docs/upgrading.md index 8bc987e..dbad32b 100644 --- a/docs/src/content/docs/upgrading.md +++ b/docs/src/content/docs/upgrading.md @@ -69,7 +69,9 @@ helm rollback nebari-apps -n nebari-apps `helm rollback` restores the workloads, RBAC and values, but not the CRD. CRD changes within `v1alpha1` are additive ([versioning policy](/app-crd-reference/#versioning-and-deprecation), -checked in CI), so the newer CRD keeps working with the older operator. The exception is +checked in CI), so the newer CRD keeps working with the older operator. Rolling back past +[gateway group enforcement](#upgrading-to-gateway-group-enforcement) brings the old sidecar +back, which also checks groups, so leave nebari-operator where it is. The exception is [group-only access](#upgrading-to-group-only-access), which removed `access.users`: rolling back does not restore the lists the newer CRD pruned. If Apps were lost, re-apply the backup: @@ -82,6 +84,63 @@ Status fields in the backup are ignored; the operator recomputes them. ## Version notes +### Upgrading to gateway group enforcement + +**Breaking:** `access.groups` are enforced at the gateway by nebari-operator +`%NEBARI_OPERATOR_VERSION%`, the first release that enforces a `NebariApp`'s `auth.groups`, and +the pack now requires it. The apps-authz sidecar no longer checks tokens or groups: it only +runs in Apps that can idle (activity for scale to zero) and in image Apps (the front port), and +a restricted App that does neither loses it. The operator's `--keycloak-url`, +`--keycloak-internal-url` and `--keycloak-realm` flags are gone; `keycloak.*` in the chart now +configures only apps-api and apps-mcp. + +- **Upgrade nebari-operator first.** With an older nebari-operator the gateway ignores + `auth.groups`, so after the pack upgrade a restricted App would be open to every signed-in + user. Upgrade nebari-operator to `%NEBARI_OPERATOR_VERSION%` (it needs Envoy Gateway v1.2.0 or + later), check that a restricted App's SecurityPolicy has an `authorization` section, then + upgrade the pack: + + ```bash + kubectl -n get securitypolicy app--security -o jsonpath='{.spec.authorization.defaultAction}{"\n"}' # Deny + ``` + + In between, the old sidecar and the gateway both check groups, which is safe. Do not roll + nebari-operator back below `%NEBARI_OPERATOR_VERSION%` while this pack runs. +- **Users sign in again once.** nebari-operator renames the access-token cookie of every App + that enforces groups (`nebari-at--`), so users of restricted Apps are signed + out once when it starts enforcing, and every App's Keycloak client is reprovisioned once. +- **The `groups` claim now carries full paths.** nebari-operator switches every client's group + mapper to full paths (`/finance`, `/finance/analysts`), and entries are matched as full + paths: `finance` is the top-level group `/finance`, and `/finance` does not admit members of + `/finance/analysts` only. Before, the claim held bare names, so an App that listed a + subgroup by its name (`analysts` for `/finance/analysts`) let its members in. Now: + - such an entry means a **top-level** group `/analysts`, which nebari-operator creates in + Keycloak if it is missing, and the subgroup's members get `403`. Replace it with the full + path (`/finance/analysts`). + - the same holds for `access.managers.groups` and the chart's `api.adminGroups`: a subgroup + named without its parent no longer matches. List its full path. + - apps that read the `groups` claim themselves (with `enforceAtGateway: false`, or the pack's + API and MCP server) see the leading `/`. apps-api and apps-mcp match full paths already. + + Find the entries to check: + + ```bash + kubectl get apps.apps.nebari.dev -A -o json | jq -r '.items[] | select((.spec.access.groups // []) + (.spec.access.managers.groups // []) | length > 0) | "\(.metadata.namespace)/\(.metadata.name): groups=\(.spec.access.groups // []) managers=\(.spec.access.managers.groups // [])"' + ``` + +- **Pods roll once.** Restricted Apps lose the sidecar unless they can idle or run an image, + and Python tasks there move from port 8081 to 8080 (`$PORT`). Every other App with the + sidecar rolls once for its new environment. +- **The token no longer reaches the app.** The gateway stopped forwarding the access token to + restricted Apps (`forwardAccessToken`). An app that read `Authorization: Bearer` there gets + no header now. +- **Keep the gateway the only way in.** The sidecar used to refuse non-members even for + traffic from inside the cluster. Turn on `apps.networkPolicy.enabled` so app pods take + traffic only from the gateway. +- **Mixed versions.** An authz image from this release refuses to start (`AUTHZ_GROUPS is set`) + next to an older apps-operator, so a restricted App never runs open; upgrade the chart and + images together. + ### Upgrading to group-only access **Breaking:** Apps restrict who may open them by Keycloak group only. `spec.access.users` diff --git a/docs/test/build.test.ts b/docs/test/build.test.ts index 800db78..9e0b926 100644 --- a/docs/test/build.test.ts +++ b/docs/test/build.test.ts @@ -95,14 +95,15 @@ test('every nebari-operator version written outside the docs matches .nebari-ope // README and agent docs are rendered by GitHub, so they carry the literal version. for (const file of ['README.md', 'AGENTS.md', 'docs/PLAN.md']) { const text = readFileSync(join(REPO, file), 'utf8'); - for (const [found] of text.matchAll(/v0\.\d+\.\d+-alpha\.\d+/g)) { + // Any v0.x release, alpha or not (the pin left the alpha line at v0.1.0). + for (const [found] of text.matchAll(/v0\.\d+\.\d+(?:-alpha\.\d+)?/g)) { expect(`${file}: ${found}`).toBe(`${file}: ${PIN}`); } } // Docs pages use the %NEBARI_OPERATOR_VERSION% placeholder instead. const docs = join(SITE, 'src', 'content', 'docs'); for (const name of readdirSync(docs)) { - expect(`${name}: ${readFileSync(join(docs, name), 'utf8').match(/v0\.\d+\.\d+-alpha\.\d+/)?.[0] ?? ''}`).toBe( + expect(`${name}: ${readFileSync(join(docs, name), 'utf8').match(/v0\.\d+\.\d+(?:-alpha\.\d+)?/)?.[0] ?? ''}`).toBe( `${name}: `, ); } diff --git a/examples/python-app/pixi.toml b/examples/python-app/pixi.toml index f2f2c77..c2752db 100644 --- a/examples/python-app/pixi.toml +++ b/examples/python-app/pixi.toml @@ -9,5 +9,5 @@ streamlit = ">=1.44" [tasks] # The platform runs `pixi run start` and expects a server on 0.0.0.0:$PORT -# (8080, or 8081 when access.groups is set or the app can idle). Locally: PORT=8080 pixi run start +# (8080, or 8081 when the app can idle). Locally: PORT=8080 pixi run start start = "streamlit run app.py --server.address 0.0.0.0 --server.port $PORT --server.headless true" diff --git a/examples/web-terminal/nebari-app.yaml b/examples/web-terminal/nebari-app.yaml index 9b5cead..178ec94 100644 --- a/examples/web-terminal/nebari-app.yaml +++ b/examples/web-terminal/nebari-app.yaml @@ -15,7 +15,7 @@ runtime: limits: { cpu: "2", memory: 2Gi } access: public: false # a shell: keep it behind Keycloak SSO at the gateway - # Only members of `admin` get in: the authz sidecar checks the access token - # the gateway forwards and returns 403 to everyone else. + # Only members of the top-level group `/admin` get in: the gateway returns + # 403 to every other signed-in user (list a subgroup by its full path). groups: ["admin"] subdomain: web-terminal diff --git a/examples/web-terminal/pixi.toml b/examples/web-terminal/pixi.toml index 70bb405..9e4dec7 100644 --- a/examples/web-terminal/pixi.toml +++ b/examples/web-terminal/pixi.toml @@ -43,7 +43,7 @@ nss_wrapper = "*" [tasks] # The platform runs `pixi run start` and expects a server on 0.0.0.0:$PORT: -# 8081 here, because access.groups puts the authz sidecar on 8080. Locally: +# 8080, or 8081 when the cluster idles apps (the authz sidecar takes 8080). Locally: # PORT=8080 pixi run start # -W makes the terminal writable (ttyd defaults to read-only). terminal.sh # runs per browser session: identity setup, then an interactive bash that diff --git a/mcp/src/nebari_apps_mcp/server.py b/mcp/src/nebari_apps_mcp/server.py index 45454e3..981c593 100644 --- a/mcp/src/nebari_apps_mcp/server.py +++ b/mcp/src/nebari_apps_mcp/server.py @@ -398,8 +398,10 @@ async def launch_app( list[str] | None, Field( description=( - "Keycloak groups allowed to open the app (private apps; enforced in the app pod, " - "everyone else gets 403). Empty = any signed-in user" + "Keycloak groups allowed to open the app (private apps; enforced at the gateway, " + "every other signed-in user gets 403). Full group paths matched exactly: 'finance' is " + "'/finance', and '/finance' does not admit members of '/finance/analysts' only, so list " + "each subgroup. Empty = any signed-in user" ) ), ] = None, @@ -417,8 +419,8 @@ async def launch_app( list[str] | None, Field( description=( - "Keycloak groups whose members may manage the app, like manager_users. " - "Omit to keep the current list when re-launching; [] clears it" + "Keycloak groups whose members may manage the app, like manager_users " + "(full group paths, matched like groups). Omit to keep the current list when re-launching; [] clears it" ) ), ] = None, @@ -910,7 +912,10 @@ def review_access( - public: true means anyone with the URL, with no sign-in. Flag this if the app looks internal or lists \ secretKeys. - public: false with empty groups means any user who can sign in to the cluster's Keycloak. - - public: false with groups is restricted: a member of any listed group gets in, everyone else gets 403. Group changes take effect when the user's access token refreshes (minutes). + - public: false with groups is restricted: the gateway lets in a member of any listed group and gives everyone \ +else 403. Entries are full group paths matched exactly ("finance" is "/finance"; "/finance" does not admit members \ +of "/finance/analysts" only), so check that each subgroup that needs access is listed. Group changes take effect \ +when the user's access token refreshes (minutes). - groups have no effect while public is true; point out that combination. 2. Who can manage it: the owner, every user in access.managers.users, members of access.managers.groups, \ and cluster admins. Managers can edit, start, stop, restart and delete the app and read its logs, but only the \ diff --git a/operator/api/v1alpha1/app_types.go b/operator/api/v1alpha1/app_types.go index 9e9184e..23a22a3 100644 --- a/operator/api/v1alpha1/app_types.go +++ b/operator/api/v1alpha1/app_types.go @@ -174,8 +174,8 @@ type AppRuntime struct { // serving its source as static content: the operator installs the pixi // environment from the app source and executes `pixi run `. // Not allowed with source type image, which runs its own server. - // The task must start a server on $PORT: 8080, or 8081 when access.groups - // restricts the app or it can idle (the authz sidecar takes 8080). + // The task must start a server on $PORT: 8080, or 8081 when the app can + // idle (the authz sidecar takes 8080). // +kubebuilder:validation:MaxLength=64 // +optional PixiTask string `json:"pixiTask,omitempty"` @@ -187,13 +187,17 @@ type AppAccess struct { // +optional Public bool `json:"public,omitempty"` - // Groups are the Keycloak/OIDC groups allowed to open a private app. - // Enforced in the app pod by the authz sidecar, which checks the access - // token the gateway forwards: everyone else gets 403. A user in any - // listed group is let in. Empty allows any signed-in realm user. Also - // forwarded to the NebariApp, where it sets landing-page tile visibility. - // Changes apply when the user's access token - // refreshes (Keycloak's access token lifespan, 5 minutes by default). + // Groups are the Keycloak groups allowed to open a private app. Enforced + // at the gateway (NebariApp auth.groups): a signed-in user in none of + // them gets 403 before the request reaches the app. A user in any listed + // group is let in. Empty allows any signed-in realm user. Entries are + // full group paths matched exactly: "finance" is "/finance", and + // "/finance" does not admit members of "/finance/analysts" only, so list + // each subgroup that needs access. A bare top-level name is created in + // Keycloak if missing; a path must already exist. The groups also set + // landing-page tile visibility. Changes apply when the user's access + // token refreshes (Keycloak's access token lifespan, 5 minutes by + // default). // +optional Groups []string `json:"groups,omitempty"` @@ -221,8 +225,9 @@ type AppManagers struct { // +optional Users []string `json:"users,omitempty"` - // Groups are Keycloak/OIDC groups whose members may manage the app - // (a leading "/" is ignored, as for api.adminGroups). + // Groups are Keycloak groups whose members may manage the app, matched + // as full group paths like access.groups ("finance" is "/finance", which + // does not include "/finance/analysts"). // +optional Groups []string `json:"groups,omitempty"` } diff --git a/operator/cmd/main.go b/operator/cmd/main.go index 96ba330..78ab51d 100644 --- a/operator/cmd/main.go +++ b/operator/cmd/main.go @@ -53,16 +53,10 @@ func main() { flag.StringVar(&cfg.PythonImage, "python-image", envOr("PYTHON_IMAGE", "ghcr.io/prefix-dev/pixi:0.68.1-noble"), "Image running Python/pixi apps (runtime.pixiTask); must provide pixi and run as non-root.") flag.StringVar(&cfg.AuthzImage, "authz-image", envOr("AUTHZ_IMAGE", ""), - "apps-authz sidecar image, added to Apps that restrict access.groups (required for those Apps).") + "apps-authz sidecar image, added to Apps that can idle and to image Apps (required for those Apps).") var authzPullPolicy string flag.StringVar(&authzPullPolicy, "authz-image-pull-policy", envOr("AUTHZ_IMAGE_PULL_POLICY", ""), "Pull policy for the authz sidecar image (Always|IfNotPresent|Never; empty: the Kubernetes default).") - flag.StringVar(&cfg.KeycloakURL, "keycloak-url", envOr("KEYCLOAK_URL", ""), - "Public Keycloak base URL; the authz sidecar accepts tokens issued under /realms/.") - flag.StringVar(&cfg.KeycloakInternalURL, "keycloak-internal-url", envOr("KEYCLOAK_INTERNAL_URL", ""), - "In-cluster Keycloak base URL; the authz sidecar also accepts it as issuer and fetches signing keys from it.") - flag.StringVar(&cfg.KeycloakRealm, "keycloak-realm", envOr("KEYCLOAK_REALM", "nebari"), - "Keycloak realm nebari-operator provisions app clients in.") flag.BoolVar(&cfg.TLSDisabled, "tls-disabled", envOr("TLS_DISABLED", "") == "true", "Serve apps over plain HTTP (no certificates, no HTTPS listeners).") flag.BoolVar(&cfg.LandingPage, "landing-page", envOr("LANDING_PAGE", "") == "true", @@ -191,7 +185,7 @@ func main() { setupLog.Info("starting manager", "appsDomain", cfg.AppsDomain, "gateway", cfg.Gateway, - "authzImage", cfg.AuthzImage, "keycloakIssuers", len(cfg.KeycloakURL+cfg.KeycloakInternalURL) > 0, + "authzImage", cfg.AuthzImage, "idleTimeout", cfg.IdleTimeout.String(), "activatorService", cfg.ActivatorNamespace+"/"+cfg.ActivatorService, "imageRegistries", cfg.ImageRegistries) if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { diff --git a/operator/internal/controller/app_controller.go b/operator/internal/controller/app_controller.go index 63fc212..5bba19f 100644 --- a/operator/internal/controller/app_controller.go +++ b/operator/internal/controller/app_controller.go @@ -58,22 +58,14 @@ type OperatorConfig struct { // limit an App leaves unset (static apps vs runtime.pixiTask apps). StaticResources corev1.ResourceRequirements PythonResources corev1.ResourceRequirements - // AuthzImage is the apps-authz sidecar added in front of Apps that - // restrict access to groups (authz.go). + // AuthzImage is the apps-authz sidecar added in front of Apps that can + // idle and of image Apps (authz.go). AuthzImage string // AuthzImagePullPolicy is the sidecar's pull policy (empty: the // Kubernetes default). AuthzImagePullPolicy corev1.PullPolicy // AuthzResources are the sidecar's requests/limits. AuthzResources corev1.ResourceRequirements - // KeycloakURL and KeycloakInternalURL are the realm's public and - // in-cluster base URLs; the sidecar accepts tokens issued under either - // and fetches keys from the in-cluster one when set. - KeycloakURL string - KeycloakInternalURL string - // KeycloakRealm is the realm nebari-operator provisions clients in - // (default nebari). - KeycloakRealm string // IdleTimeout is the cluster default for scaling an App to zero after // it goes this long without a request (0: Apps never idle unless they // set runtime.idleTimeoutMinutes). See idle.go. diff --git a/operator/internal/controller/app_controller_test.go b/operator/internal/controller/app_controller_test.go index be38ad4..91d4950 100644 --- a/operator/internal/controller/app_controller_test.go +++ b/operator/internal/controller/app_controller_test.go @@ -73,18 +73,15 @@ func newReconciler(t *testing.T, objs ...client.Object) (*AppReconciler, client. Client: c, Scheme: s, Config: OperatorConfig{ - AppsDomain: testDomain, - Gateway: "public", - StaticImage: "nginxinc/nginx-unprivileged:1.27-alpine", - GitImage: "alpine/git:v2.47.2", - PythonImage: "ghcr.io/prefix-dev/pixi:0.68.1-noble", - StaticResources: DefaultStaticResources, - PythonResources: DefaultPythonResources, - AuthzImage: testAuthzImage, - AuthzResources: DefaultAuthzResources, - KeycloakURL: "https://keycloak.nebari.local", - KeycloakInternalURL: "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080", - KeycloakRealm: "nebari", + AppsDomain: testDomain, + Gateway: "public", + StaticImage: "nginxinc/nginx-unprivileged:1.27-alpine", + GitImage: "alpine/git:v2.47.2", + PythonImage: "ghcr.io/prefix-dev/pixi:0.68.1-noble", + StaticResources: DefaultStaticResources, + PythonResources: DefaultPythonResources, + AuthzImage: testAuthzImage, + AuthzResources: DefaultAuthzResources, }, } return r, c diff --git a/operator/internal/controller/authz.go b/operator/internal/controller/authz.go index cd9ffaf..83fd277 100644 --- a/operator/internal/controller/authz.go +++ b/operator/internal/controller/authz.go @@ -12,12 +12,13 @@ import ( appsv1alpha1 "github.com/nebari-dev/nebari-apps-pack/operator/api/v1alpha1" ) -// The gateway's SecurityPolicy only signs users in (nebari-operator #153), so -// an App that lists access.groups gets the apps-authz -// sidecar: it takes AppPort, checks the access token the gateway forwards -// (NebariApp auth.forwardAccessToken) and proxies allowed requests to the -// app container on AppInternalPort. Unrestricted Apps get no sidecar and an -// unchanged pod spec. +// The apps-authz sidecar sits in front of the app container of Apps that can +// idle (it records request activity for the idle check, idle.go) and of image +// Apps (it gives the image's own port the pack's one front port, AppPort). It +// takes AppPort and proxies every request to the app container on +// appContainerPort: it checks no tokens. access.groups are enforced by the +// gateway (routing.go emits them as the NebariApp's auth.groups), so other +// Apps get no sidecar and an unchanged pod spec. const ( // AppInternalPort is where the app container listens behind the sidecar. AppInternalPort = 8081 @@ -43,18 +44,17 @@ var DefaultAuthzResources = corev1.ResourceRequirements{ } // restricted reports whether an App limits who may open it beyond "any -// signed-in user": a private App with groups. +// signed-in user": a private App with groups. The gateway enforces them. func restricted(app *appsv1alpha1.App) bool { a := app.Spec.Access return !a.Public && len(a.Groups) > 0 } -// hasSidecar reports whether the App's pod runs the apps-authz sidecar: -// restricted Apps (it enforces groups), Apps that can idle (in -// passthrough mode it only records activity for the idle check) and image -// Apps (it gives their own port the pack's one front port, AppPort). +// hasSidecar reports whether the App's pod runs the apps-authz sidecar: Apps +// that can idle (it records activity for the idle check) and image Apps (it +// gives their own port the pack's one front port, AppPort). func (c OperatorConfig) hasSidecar(app *appsv1alpha1.App) bool { - return restricted(app) || c.idleTimeout(app) > 0 || isImage(app) + return c.idleTimeout(app) > 0 || isImage(app) } func isImage(app *appsv1alpha1.App) bool { @@ -73,81 +73,45 @@ func (c OperatorConfig) appContainerPort(app *appsv1alpha1.App) int32 { return AppPort } -// oidcClientID is the Keycloak client nebari-operator provisions for the -// App's NebariApp, which the gateway logs users in with (the token's azp). -// It mirrors nebari-operator's naming.ClientID, "-"; if that naming changes, every restricted App denies everyone -// (TestAuthzClientIDMatchesNebariOperator pins it). -func oidcClientID(app *appsv1alpha1.App) string { - return app.Namespace + "-" + childName(app) -} - -func realmURL(base, realm string) string { - if base == "" { - return "" - } - return strings.TrimSuffix(base, "/") + "/realms/" + realm -} - -// authzIssuers are the iss values the sidecar accepts. Keycloak writes the -// URL its token endpoint was called at, which can be the public or the -// in-cluster URL depending on its hostname settings. -func (c OperatorConfig) authzIssuers() []string { - var out []string - for _, base := range []string{c.KeycloakURL, c.KeycloakInternalURL} { - if u := realmURL(base, c.realm()); u != "" && !contains(out, u) { - out = append(out, u) - } - } - return out -} - -// authzJWKSURL fetches the realm's keys in-cluster when possible. -func (c OperatorConfig) authzJWKSURL() string { - base := c.KeycloakInternalURL - if base == "" { - base = c.KeycloakURL - } - return realmURL(base, c.realm()) + "/protocol/openid-connect/certs" -} - -func (c OperatorConfig) realm() string { - if c.KeycloakRealm == "" { - return "nebari" - } - return c.KeycloakRealm -} - -// checkAuthz reports why a restricted App cannot be enforced, so it fails -// validation instead of running open. +// checkAuthz reports why an App cannot run as specified, so it fails +// validation instead: an image App needs the sidecar, and every access.groups +// entry must name a group (nebari-operator denies every request when all +// entries normalize to nothing, such as "/"). func (c OperatorConfig) checkAuthz(app *appsv1alpha1.App) error { if isImage(app) && c.AuthzImage == "" { return fmt.Errorf("image sources need the authz sidecar, but the operator has no --authz-image (chart authz.image)") } - if !restricted(app) { - return nil - } - if c.AuthzImage == "" { - return fmt.Errorf("access.groups need the authz sidecar, but the operator has no --authz-image (chart authz.image)") - } - if len(c.authzIssuers()) == 0 { - return fmt.Errorf("access.groups need the realm's URL, but the operator has no --keycloak-url or --keycloak-internal-url (chart keycloak.url, keycloak.internalUrl)") - } - for _, v := range app.Spec.Access.Groups { - if strings.TrimSpace(v) == "" || strings.Contains(v, ",") { - return fmt.Errorf("access.groups entries must be non-empty and must not contain ','") + for _, g := range app.Spec.Access.Groups { + if groupPath(g) == "" { + return fmt.Errorf("access.groups entries must name a group, such as finance or /parent/child") } } return nil } +// groupPath is nebari-operator's canonical form of an access.groups entry +// (its groups.NormalizePath): the full Keycloak group path, with a leading +// "/", no trailing "/" and no empty segments, so "finance" and "/finance" are +// the same group. It returns "" for an entry that names no group. +func groupPath(g string) string { + var segments []string + for _, s := range strings.Split(strings.TrimSpace(g), "/") { + if s != "" { + segments = append(segments, s) + } + } + if len(segments) == 0 { + return "" + } + return "/" + strings.Join(segments, "/") +} + // addAuthzSidecar moves the app container to appContainerPort (AppInternalPort, // or an image App's own port), points its // probes at the sidecar's /upstreamz (which dials the app on loopback, so an // app listening only on 127.0.0.1 still passes) and appends the sidecar, -// which takes the "http" port the Service targets. For an unrestricted App -// (one that is here only because it can idle) the sidecar runs in -// passthrough mode: it checks no tokens and only records activity. +// which takes the "http" port the Service targets. The sidecar checks no +// tokens: it proxies every request and records activity. func (r *AppReconciler) addAuthzSidecar(app *appsv1alpha1.App, pod *corev1.PodSpec) { main := &pod.Containers[0] upstreamPort := r.Config.appContainerPort(app) @@ -176,16 +140,6 @@ func (r *AppReconciler) addAuthzSidecar(app *appsv1alpha1.App, pod *corev1.PodSp {Name: "AUTHZ_HEALTH_LISTEN", Value: fmt.Sprintf(":%d", authzHealthPort)}, {Name: "AUTHZ_UPSTREAM", Value: fmt.Sprintf("http://127.0.0.1:%d", upstreamPort)}, } - if restricted(app) { - env = append(env, - corev1.EnvVar{Name: "AUTHZ_ISSUERS", Value: strings.Join(r.Config.authzIssuers(), ",")}, - corev1.EnvVar{Name: "AUTHZ_JWKS_URL", Value: r.Config.authzJWKSURL()}, - corev1.EnvVar{Name: "AUTHZ_CLIENT_ID", Value: oidcClientID(app)}, - corev1.EnvVar{Name: "AUTHZ_GROUPS", Value: strings.Join(app.Spec.Access.Groups, ",")}, - ) - } else { - env = append(env, corev1.EnvVar{Name: "AUTHZ_PASSTHROUGH", Value: "true"}) - } pod.Containers = append(pod.Containers, corev1.Container{ Name: authzContainer, @@ -195,10 +149,8 @@ func (r *AppReconciler) addAuthzSidecar(app *appsv1alpha1.App, pod *corev1.PodSp {Name: "http", ContainerPort: AppPort, Protocol: corev1.ProtocolTCP}, {Name: authzHealthName, ContainerPort: authzHealthPort, Protocol: corev1.ProtocolTCP}, }, - Env: env, - Resources: *r.Config.AuthzResources.DeepCopy(), - // Ready once the realm's signing keys are fetched, so a wrong - // Keycloak URL never takes traffic (at once in passthrough). + Env: env, + Resources: *r.Config.AuthzResources.DeepCopy(), ReadinessProbe: &corev1.Probe{ProbeHandler: healthGet("/readyz"), PeriodSeconds: 5}, LivenessProbe: &corev1.Probe{ProbeHandler: healthGet("/healthz"), PeriodSeconds: 10}, SecurityContext: &corev1.SecurityContext{ @@ -211,12 +163,3 @@ func (r *AppReconciler) addAuthzSidecar(app *appsv1alpha1.App, pod *corev1.PodSp }, }) } - -func contains(list []string, s string) bool { - for _, v := range list { - if v == s { - return true - } - } - return false -} diff --git a/operator/internal/controller/authz_test.go b/operator/internal/controller/authz_test.go index 11b94ef..f0b2734 100644 --- a/operator/internal/controller/authz_test.go +++ b/operator/internal/controller/authz_test.go @@ -30,9 +30,11 @@ func envOf(c corev1.Container) map[string]string { return out } +// deployed reconciles app once on a cluster where Apps idle after 30 minutes +// (so every App runs the sidecar) and returns its children. func deployed(t *testing.T, app *appsv1alpha1.App) (*appsv1.Deployment, *corev1.Service, *corev1.ConfigMap, *AppReconciler) { t.Helper() - r, c := newReconciler(t, managedNamespace(app.Namespace), app) + r, c, _ := idleReconciler(t, newFakePodActivity(t), managedNamespace(app.Namespace), app) reconcile(t, r, app) if got := getApp(t, c, app.Name, app.Namespace); got.Status.Phase == appsv1alpha1.AppPhaseFailed { t.Fatalf("app failed: %s", got.Status.Message) @@ -53,10 +55,19 @@ func deployed(t *testing.T, app *appsv1alpha1.App) (*appsv1.Deployment, *corev1. return d, svc, cm, r } +// The sidecar only proxies: the environment carries no token or group +// settings, even for an App that lists access.groups (the gateway enforces +// them). +var passthroughEnv = map[string]string{ + "AUTHZ_LISTEN": ":8080", + "AUTHZ_HEALTH_LISTEN": ":8082", + "AUTHZ_UPSTREAM": "http://127.0.0.1:8081", +} + func TestAuthzSidecarStaticApp(t *testing.T) { app := restrictedApp(inlineApp("apps"), []string{"analysts", "admin"}) app.Name = "web-terminal" - d, svc, cm, _ := deployed(t, app) + d, svc, cm, r := deployed(t, app) pod := d.Spec.Template.Spec if len(pod.Containers) != 2 || pod.Containers[0].Name != "app" || pod.Containers[1].Name != "authz" { @@ -78,8 +89,8 @@ func TestAuthzSidecarStaticApp(t *testing.T) { if conf := cm.Data["default.conf"]; !strings.Contains(conf, "listen 127.0.0.1:8081;") { t.Errorf("nginx should listen on loopback only:\n%s", conf) } - if d.Spec.Template.Annotations["apps.nebari.dev/server-config-checksum"] != (OperatorConfig{}).serverConfChecksum(app) { - t.Error("server-config checksum does not follow the restricted config") + if d.Spec.Template.Annotations["apps.nebari.dev/server-config-checksum"] != r.Config.serverConfChecksum(app) { + t.Error("server-config checksum does not follow the sidecar config") } // The Service and NetworkPolicy keep targeting 8080 ("http"), now the sidecar. @@ -90,18 +101,8 @@ func TestAuthzSidecarStaticApp(t *testing.T) { t.Errorf("sidecar ports = %+v", side.Ports) } - want := map[string]string{ - "AUTHZ_LISTEN": ":8080", - "AUTHZ_HEALTH_LISTEN": ":8082", - "AUTHZ_UPSTREAM": "http://127.0.0.1:8081", - "AUTHZ_ISSUERS": "https://keycloak.nebari.local/realms/nebari," + - "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080/realms/nebari", - "AUTHZ_JWKS_URL": "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080/realms/nebari/protocol/openid-connect/certs", - "AUTHZ_CLIENT_ID": "apps-app-web-terminal", - "AUTHZ_GROUPS": "analysts,admin", - } - if got := envOf(side); !reflect.DeepEqual(got, want) { - t.Errorf("sidecar env =\n%v\nwant\n%v", got, want) + if got := envOf(side); !reflect.DeepEqual(got, passthroughEnv) { + t.Errorf("sidecar env =\n%v\nwant\n%v", got, passthroughEnv) } if side.Image != testAuthzImage { t.Errorf("sidecar image = %q", side.Image) @@ -118,7 +119,7 @@ func TestAuthzSidecarStaticApp(t *testing.T) { t.Errorf("sidecar probes = %+v %+v", side.ReadinessProbe, side.LivenessProbe) } if *pod.AutomountServiceAccountToken { - t.Error("restricted pods must not mount a ServiceAccount token") + t.Error("app pods must not mount a ServiceAccount token") } } @@ -141,15 +142,15 @@ func TestAuthzSidecarPixiApp(t *testing.T) { if main.StartupProbe.FailureThreshold != 60 { t.Error("the cold-start budget of the startup probe should be kept") } - env := envOf(pod.Containers[1]) - if env["AUTHZ_GROUPS"] != "analysts" { - t.Errorf("AUTHZ_GROUPS = %q", env["AUTHZ_GROUPS"]) + if got := envOf(pod.Containers[1]); !reflect.DeepEqual(got, passthroughEnv) { + t.Errorf("sidecar env = %v, want %v", got, passthroughEnv) } } -// Apps that do not restrict access get exactly the pod they got before the -// sidecar existed, whatever the authz settings. -func TestUnrestrictedAppsHaveNoSidecar(t *testing.T) { +// Apps that neither idle nor run an image get exactly the pod they got before +// the sidecar existed, whatever the authz settings, restricted ones included: +// the gateway enforces access.groups. +func TestAppsThatNeitherIdleNorRunAnImageHaveNoSidecar(t *testing.T) { publicWithLists := inlineApp("team-a") publicWithLists.Spec.Access.Groups = []string{"x"} private := inlineApp("team-a") @@ -160,6 +161,8 @@ func TestUnrestrictedAppsHaveNoSidecar(t *testing.T) { for name, app := range map[string]*appsv1alpha1.App{ "public": inlineApp("team-a"), "public with lists": publicWithLists, "private": private, "private pixi": privatePixi, + "restricted": restrictedApp(inlineApp("team-a"), []string{"analysts"}), + "restricted pixi": restrictedApp(pixiApp("team-a"), []string{"/ops/oncall"}), } { t.Run(name, func(t *testing.T) { r, _ := newReconciler(t) @@ -168,13 +171,13 @@ func TestUnrestrictedAppsHaveNoSidecar(t *testing.T) { t.Fatal(err) } bare := *r - bare.Config.AuthzImage, bare.Config.KeycloakURL, bare.Config.KeycloakInternalURL = "", "", "" + bare.Config.AuthzImage = "" without, err := bare.buildDeployment(app) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(withAuthz, without) { - t.Error("the authz settings changed an unrestricted App's Deployment") + t.Error("the authz settings changed the App's Deployment") } pod := withAuthz.Spec.Template.Spec if len(pod.Containers) != 1 || pod.Containers[0].Ports[0].Name != "http" || pod.Containers[0].Ports[0].ContainerPort != AppPort { @@ -184,10 +187,11 @@ func TestUnrestrictedAppsHaveNoSidecar(t *testing.T) { t.Error("nginx should listen on 8080") } auth, _, _ := unstructured.NestedMap(r.buildNebariApp(app).Object, "spec", "auth") - for _, k := range []string{"forwardAccessToken", "enforceAtGateway", "users"} { - if _, has := auth[k]; has { - t.Errorf("auth.%s set on an unrestricted App", k) - } + if _, has := auth["forwardAccessToken"]; has { + t.Error("auth.forwardAccessToken set: no sidecar reads the token any more") + } + if _, has := auth["users"]; has { + t.Error("auth.users should never be emitted") } }) } @@ -195,18 +199,20 @@ func TestUnrestrictedAppsHaveNoSidecar(t *testing.T) { func TestAuthzMisconfigurationFailsValidation(t *testing.T) { cases := map[string]struct { - edit func(*OperatorConfig) - groups []string - want string + app *appsv1alpha1.App + edit func(*OperatorConfig) + want string }{ - "no image": {func(c *OperatorConfig) { c.AuthzImage = "" }, []string{"a"}, "--authz-image"}, - "no keycloak": {func(c *OperatorConfig) { c.KeycloakURL, c.KeycloakInternalURL = "", "" }, []string{"a"}, "--keycloak-url"}, - "comma in group": {func(*OperatorConfig) {}, []string{"a,b"}, "must not contain ','"}, - "blank group": {func(*OperatorConfig) {}, []string{" "}, "must be non-empty"}, + "image without sidecar image": {imageApp("team-a", "nginx:1.27"), func(c *OperatorConfig) { + c.ImageRegistries = []string{"docker.io/library"} + c.AuthzImage = "" + }, "--authz-image"}, + "blank group": {restrictedApp(inlineApp("team-a"), []string{" "}), func(*OperatorConfig) {}, "must name a group"}, + "root group": {restrictedApp(inlineApp("team-a"), []string{"analysts", "/"}), func(*OperatorConfig) {}, "must name a group"}, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - app := restrictedApp(inlineApp("team-a"), tc.groups) + app := tc.app r, c := newReconciler(t, managedNamespace("team-a"), app) tc.edit(&r.Config) reconcile(t, r, app) @@ -215,36 +221,28 @@ func TestAuthzMisconfigurationFailsValidation(t *testing.T) { t.Errorf("phase %s message %q, want Failed with %q", got.Status.Phase, got.Status.Message, tc.want) } if err := c.Get(context.Background(), types.NamespacedName{Name: childName(app), Namespace: "team-a"}, &appsv1.Deployment{}); err == nil { - t.Error("a Deployment was created for an App whose access cannot be enforced") + t.Error("a Deployment was created for an App that failed validation") } }) } } -func TestAuthzKeycloakURLs(t *testing.T) { - c := OperatorConfig{KeycloakURL: "https://kc.example.ai/auth/", KeycloakRealm: "r"} - if got := c.authzIssuers(); !reflect.DeepEqual(got, []string{"https://kc.example.ai/auth/realms/r"}) { - t.Errorf("issuers = %v", got) - } - if got := c.authzJWKSURL(); got != "https://kc.example.ai/auth/realms/r/protocol/openid-connect/certs" { - t.Errorf("jwks = %q (public URL when there is no internal one)", got) - } - c = OperatorConfig{KeycloakURL: "http://kc", KeycloakInternalURL: "http://kc"} - if got := c.authzIssuers(); !reflect.DeepEqual(got, []string{"http://kc/realms/nebari"}) { - t.Errorf("issuers = %v, want one deduplicated issuer in the default realm", got) - } -} - -// The sidecar binds tokens to the client nebari-operator provisions for the -// NebariApp: naming.ClientID is "-" (live example: -// apps-app-web-terminal). If nebari-operator changes that, restricted Apps -// deny everyone; update oidcClientID with the contract pin. -func TestAuthzClientIDMatchesNebariOperator(t *testing.T) { - app := inlineApp("apps") - app.Name = "web-terminal" - na := testReconciler().buildNebariApp(app) - if got, want := oidcClientID(app), na.GetNamespace()+"-"+na.GetName(); got != want || got != "apps-app-web-terminal" { - t.Errorf("client ID = %q, want %q", got, want) +// groupPath mirrors nebari-operator's groups.NormalizePath (v0.2.0), which +// decides what the gateway matches. +func TestGroupPath(t *testing.T) { + for in, want := range map[string]string{ + "finance": "/finance", + "/finance": "/finance", + " /finance/ ": "/finance", + "/ops//oncall": "/ops/oncall", + "parent/child": "/parent/child", + "/": "", + " ": "", + "a,b": "/a,b", + } { + if got := groupPath(in); got != want { + t.Errorf("groupPath(%q) = %q, want %q", in, got, want) + } } } diff --git a/operator/internal/controller/envtest_test.go b/operator/internal/controller/envtest_test.go index 3466672..994addc 100644 --- a/operator/internal/controller/envtest_test.go +++ b/operator/internal/controller/envtest_test.go @@ -82,8 +82,6 @@ func startEnv(t *testing.T) client.Client { PythonResources: DefaultPythonResources, AuthzImage: testAuthzImage, AuthzResources: DefaultAuthzResources, - KeycloakURL: "https://keycloak.nebari.local", - KeycloakRealm: "nebari", }, } if err := r.SetupWithManager(mgr); err != nil { @@ -307,9 +305,10 @@ func TestEnvtestCRDEnvReferences(t *testing.T) { } } -// Restricting an App adds the authz sidecar and forwards the access token; -// lifting the restriction takes both away again. -func TestEnvtestRestrictedAppGetsAuthzSidecar(t *testing.T) { +// Restricting an App hands its groups to the gateway (NebariApp auth.groups +// with enforceAtGateway) and adds no sidecar; lifting the restriction takes +// the groups away again. +func TestEnvtestRestrictedAppEnforcedAtGateway(t *testing.T) { c := startEnv(t) ctx := context.Background() if err := c.Create(ctx, managedNamespace("team-a")); err != nil { @@ -326,7 +325,7 @@ func TestEnvtestRestrictedAppGetsAuthzSidecar(t *testing.T) { child := types.NamespacedName{Name: childName(app), Namespace: app.Namespace} eventually(t, "Deploying", phaseIs(c, key, appsv1alpha1.AppPhaseDeploying)) - state := func(wantSidecar bool) func() string { + state := func(wantGroups bool) func() string { return func() string { d := &appsv1.Deployment{} if err := c.Get(ctx, child, d); err != nil { @@ -337,16 +336,19 @@ func TestEnvtestRestrictedAppGetsAuthzSidecar(t *testing.T) { if err := c.Get(ctx, child, na); err != nil { return err.Error() } - fwd, _, _ := unstructured.NestedBool(na.Object, "spec", "auth", "forwardAccessToken") + groups, _, _ := unstructured.NestedStringSlice(na.Object, "spec", "auth", "groups") + enforce, _, _ := unstructured.NestedBool(na.Object, "spec", "auth", "enforceAtGateway") + _, fwd, _ := unstructured.NestedBool(na.Object, "spec", "auth", "forwardAccessToken") containers := d.Spec.Template.Spec.Containers - hasSidecar := len(containers) == 2 && containers[1].Name == "authz" - if hasSidecar != wantSidecar || fwd != wantSidecar || containers[0].Name != "app" { - return fmt.Sprintf("containers %d, forwardAccessToken %v", len(containers), fwd) + hasGroups := len(groups) == 1 && groups[0] == "analysts" && enforce + if len(containers) != 1 || containers[0].Name != "app" || fwd || hasGroups != wantGroups { + return fmt.Sprintf("containers %d, groups %v, enforceAtGateway %v, forwardAccessToken set %v", + len(containers), groups, enforce, fwd) } return "" } } - eventually(t, "sidecar and forwarded token", state(true)) + eventually(t, "groups enforced at the gateway, no sidecar", state(true)) edit := &appsv1alpha1.App{} if err := c.Get(ctx, key, edit); err != nil { @@ -356,7 +358,7 @@ func TestEnvtestRestrictedAppGetsAuthzSidecar(t *testing.T) { if err := c.Update(ctx, edit); err != nil { t.Fatal(err) } - eventually(t, "no sidecar once unrestricted", state(false)) + eventually(t, "no groups once unrestricted", state(false)) } // setRuntimeRules swaps the CEL rules on spec.runtime in the installed App diff --git a/operator/internal/controller/idle_test.go b/operator/internal/controller/idle_test.go index 638aaad..386bb0b 100644 --- a/operator/internal/controller/idle_test.go +++ b/operator/internal/controller/idle_test.go @@ -175,8 +175,8 @@ func TestIdleAppGetsPassthroughSidecar(t *testing.T) { for _, e := range pod.Containers[1].Env { env[e.Name] = e.Value } - if env["AUTHZ_PASSTHROUGH"] != "true" || env["AUTHZ_ISSUERS"] != "" || env["AUTHZ_GROUPS"] != "" { - t.Errorf("an unrestricted App's sidecar must be passthrough only: %v", env) + if env["AUTHZ_ISSUERS"] != "" || env["AUTHZ_GROUPS"] != "" || len(env) != 3 { + t.Errorf("the sidecar must be passthrough only: %v", env) } for _, e := range pod.Containers[0].Env { if e.Name == "PORT" && e.Value != fmt.Sprint(AppInternalPort) { diff --git a/operator/internal/controller/image_test.go b/operator/internal/controller/image_test.go index ba03fa6..f285bdf 100644 --- a/operator/internal/controller/image_test.go +++ b/operator/internal/controller/image_test.go @@ -113,7 +113,7 @@ func TestReconcileImageApp(t *testing.T) { t.Errorf("readiness probe should go through the sidecar: %+v", main.ReadinessProbe) } side := envOf(pod.Containers[1]) - if side["AUTHZ_UPSTREAM"] != "http://127.0.0.1:8501" || side["AUTHZ_PASSTHROUGH"] != "true" { + if side["AUTHZ_UPSTREAM"] != "http://127.0.0.1:8501" || len(side) != 3 { t.Errorf("sidecar env = %v", side) } if pod.Containers[1].Ports[0].Name != "http" || pod.Containers[1].Ports[0].ContainerPort != AppPort { @@ -138,8 +138,10 @@ func TestImageAppRestrictedAndDigest(t *testing.T) { if *pod.SecurityContext.RunAsUser != 1000 || !*pod.SecurityContext.RunAsNonRoot { t.Errorf("security context = %+v", pod.SecurityContext) } + // The sidecar still fronts the image's port; the gateway enforces the + // groups, so it gets no group settings. side := envOf(pod.Containers[1]) - if side["AUTHZ_GROUPS"] != "analysts" || side["AUTHZ_PASSTHROUGH"] != "" { + if side["AUTHZ_UPSTREAM"] != "http://127.0.0.1:8501" || side["AUTHZ_GROUPS"] != "" { t.Errorf("restricted image app sidecar env = %v", side) } } diff --git a/operator/internal/controller/nebariapp_contract_test.go b/operator/internal/controller/nebariapp_contract_test.go index 554488e..29bfbcf 100644 --- a/operator/internal/controller/nebariapp_contract_test.go +++ b/operator/internal/controller/nebariapp_contract_test.go @@ -97,11 +97,14 @@ func TestNebariAppContractPrivateApp(t *testing.T) { if len(groups) != 1 || groups[0] != "analytics" { t.Errorf("auth.groups = %+v", groups) } - // Groups are enforced by the authz sidecar on the forwarded token, which - // needs forwardAccessToken (and enforceAtGateway, per the CRD's rule). - if auth["forwardAccessToken"] != true || auth["enforceAtGateway"] != true { - t.Errorf("auth.forwardAccessToken/enforceAtGateway = %v/%v, want true/true", - auth["forwardAccessToken"], auth["enforceAtGateway"]) + // The gateway enforces the groups (nebari-operator v0.2.0+): a signed-in + // non-member gets 403 there. No sidecar reads the token, so it is not + // forwarded to the (untrusted) app. + if auth["enforceAtGateway"] != true { + t.Errorf("auth.enforceAtGateway = %v, want true", auth["enforceAtGateway"]) + } + if _, has := auth["forwardAccessToken"]; has { + t.Errorf("auth.forwardAccessToken = %v, want unset", auth["forwardAccessToken"]) } scopes := auth["scopes"].([]any) want := []string{"openid", "profile", "email", "groups"} @@ -123,18 +126,26 @@ func TestNebariAppContractPrivateApp(t *testing.T) { } } -// A group-restricted App forwards the token for the sidecar and never emits -// an auth.users field. -func TestNebariAppContractGroupsOnly(t *testing.T) { +// A group-restricted App hands its entries to the gateway as written: +// nebari-operator normalizes them to full group paths ("analysts" is +// "/analysts") and matches them exactly, so a subgroup path passes through +// unchanged. It never emits auth.users or forwards the token. +func TestNebariAppContractGroupsEnforcedAtGateway(t *testing.T) { app := inlineApp("team-a") - app.Spec.Access = appsv1alpha1.AppAccess{Groups: []string{"analysts"}, Subdomain: "docs-site"} + app.Spec.Access = appsv1alpha1.AppAccess{Groups: []string{"analysts", "/ops/oncall"}, Subdomain: "docs-site"} auth, _, _ := unstructured.NestedMap(testReconciler().buildNebariApp(app).Object, "spec", "auth") - if auth["enabled"] != true || auth["forwardAccessToken"] != true || auth["enforceAtGateway"] != true { - t.Errorf("auth = %+v, want sign-in with the token forwarded", auth) + if auth["enabled"] != true || auth["provider"] != "keycloak" || auth["enforceAtGateway"] != true { + t.Errorf("auth = %+v, want keycloak sign-in enforced at the gateway", auth) + } + groups, _, _ := unstructured.NestedStringSlice(auth, "groups") + if len(groups) != 2 || groups[0] != "analysts" || groups[1] != "/ops/oncall" { + t.Errorf("auth.groups = %v, want [analysts /ops/oncall]", groups) } - if _, has := auth["users"]; has { - t.Error("auth.users should never be emitted") + for _, k := range []string{"users", "forwardAccessToken"} { + if _, has := auth[k]; has { + t.Errorf("auth.%s should not be emitted", k) + } } } @@ -250,3 +261,22 @@ func TestNebariAppContractIdleRoutesToActivator(t *testing.T) { t.Errorf("auth changed: %+v -> %+v", before, after) } } + +// A restricted App keeps its gateway group rule while it idles, so the +// activator's wake page is only shown to members. +func TestNebariAppContractIdleRestrictedKeepsGroups(t *testing.T) { + app := inlineApp("team-analytics") + app.Spec.Access.Public = false + app.Spec.Access.Groups = []string{"analysts"} + r := testReconciler() + r.Config.ActivatorService = "nebari-apps-activator" + r.Config.ActivatorNamespace = "nebari-apps" + na := r.buildNebariApp(app) + r.Config.routeToActivator(na) + + groups, _, _ := unstructured.NestedStringSlice(na.Object, "spec", "auth", "groups") + enforce, _, _ := unstructured.NestedBool(na.Object, "spec", "auth", "enforceAtGateway") + if len(groups) != 1 || groups[0] != "analysts" || !enforce { + t.Errorf("auth.groups = %v enforceAtGateway = %v, want [analysts] true", groups, enforce) + } +} diff --git a/operator/internal/controller/phase_test.go b/operator/internal/controller/phase_test.go index d921acd..b112c4e 100644 --- a/operator/internal/controller/phase_test.go +++ b/operator/internal/controller/phase_test.go @@ -147,7 +147,7 @@ func TestPhaseRunningToDeployingWhenRoutingDrops(t *testing.T) { reconcile(t, r, app) got := wantPhase(t, c, app, appsv1alpha1.AppPhaseDeploying) cond := meta.FindStatusCondition(got.Status.Conditions, appsv1alpha1.ConditionRoutingReady) - if cond == nil || cond.Reason != "NebariAppNotReady" || cond.Message != "routing False" { + if cond == nil || cond.Reason != "NebariAppNotReady" || cond.Message != "Test: routing False" { t.Errorf("RoutingReady should carry the NebariApp's reason and message: %+v", cond) } } diff --git a/operator/internal/controller/routing.go b/operator/internal/controller/routing.go index 20a1f58..17b623d 100644 --- a/operator/internal/controller/routing.go +++ b/operator/internal/controller/routing.go @@ -12,7 +12,7 @@ import ( // NebariOperatorContract is the nebari-operator release whose NebariApp shape // this pack emits. It must equal the repo's .nebari-operator-version (the dev // cluster and docs read that file); TestContractVersionIsTheRepoPin checks. -const NebariOperatorContract = "v0.1.0-alpha.20" +const NebariOperatorContract = "v0.2.0" // NebariAppGVK is the nebari-operator CRD this pack emits for routing, TLS, // SSO, and landing-page registration (see NebariOperatorContract). @@ -38,22 +38,19 @@ func (r *AppReconciler) buildNebariApp(app *appsv1alpha1.App) *unstructured.Unst auth["provider"] = "keycloak" auth["provisionClient"] = true auth["scopes"] = []any{"openid", "profile", "email", "groups"} - if len(app.Spec.Access.Groups) > 0 { + if restricted(app) { + // The gateway enforces the groups (nebari-operator v0.2.0+): + // a signed-in user outside them gets 403 before the request + // reaches the pod. nebari-operator matches entries as full + // group paths ("finance" is "/finance"). enforceAtGateway + // defaults to true; it is set explicitly so a change of that + // default can never open a restricted App. groups := make([]any, 0, len(app.Spec.Access.Groups)) for _, g := range app.Spec.Access.Groups { groups = append(groups, g) } auth["groups"] = groups - } - // NebariApp's groups only drive landing-tile visibility; the authz - // sidecar is what enforces them. - if restricted(app) { - // The authz sidecar (authz.go) enforces the groups on the - // access token the gateway forwards. forwardAccessToken requires - // enforceAtGateway (a CEL rule on the NebariApp CRD), which - // defaults to true; it is set explicitly so the rule holds. auth["enforceAtGateway"] = true - auth["forwardAccessToken"] = true } } diff --git a/operator/internal/controller/secrets_test.go b/operator/internal/controller/secrets_test.go index 4ce8be2..747a63d 100644 --- a/operator/internal/controller/secrets_test.go +++ b/operator/internal/controller/secrets_test.go @@ -4,6 +4,7 @@ import ( "context" "strings" "testing" + "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -38,10 +39,10 @@ func TestSecretKeysInjectFromTheAppsOwnSecret(t *testing.T) { app := tc.app app.Spec.Runtime.Env = []corev1.EnvVar{{Name: "MODE", Value: "prod"}} app.Spec.Runtime.SecretKeys = []string{"API_KEY", "DATABASE_URL"} - // Restricted, so the pod also runs the authz sidecar. - app.Spec.Access.Public = false - app.Spec.Access.Groups = []string{"team"} + // Can idle, so the pod also runs the authz sidecar. r, _ := newReconciler(t) + r.Config.IdleTimeout = 30 * time.Minute + r.Config.ActivatorService, r.Config.ActivatorNamespace = activatorSvc, activatorNS deploy, err := r.buildDeployment(app) if err != nil { t.Fatal(err) diff --git a/operator/internal/controller/status.go b/operator/internal/controller/status.go index 19709db..0f4d256 100644 --- a/operator/internal/controller/status.go +++ b/operator/internal/controller/status.go @@ -141,7 +141,12 @@ func (r *AppReconciler) readNebariAppReady(ctx context.Context, app *appsv1alpha "NebariAppReady", "routing, TLS, and auth are configured") return true } + // The NebariApp's reason names the cause, such as + // GroupsNotResolved (an access.groups path missing in Keycloak). msg, _ := cond["message"].(string) + if reason, _ := cond["reason"].(string); reason != "" { + msg = reason + ": " + msg + } setCondition(app, appsv1alpha1.ConditionRoutingReady, metav1.ConditionFalse, "NebariAppNotReady", msg) return false diff --git a/operator/internal/controller/workload.go b/operator/internal/controller/workload.go index 1c00785..f2ad5c4 100644 --- a/operator/internal/controller/workload.go +++ b/operator/internal/controller/workload.go @@ -14,9 +14,10 @@ import ( const ( // AppPort is the port the Service and the chart's NetworkPolicy target. // The app container listens on it, unless the App runs the authz sidecar - // (it restricts access to groups, can idle, or runs an image): then the - // sidecar takes it and the app moves to AppInternalPort (authz.go). Sign-in is enforced at the gateway by the - // NebariApp SecurityPolicy, so apps serve plain HTTP here. + // (it can idle, or runs an image): then the sidecar takes it and the app + // moves to AppInternalPort (authz.go). Sign-in and access.groups are + // enforced at the gateway by the NebariApp SecurityPolicy, so apps serve + // plain HTTP here. AppPort = 8080 // webRoot is where the static server image serves content from. diff --git a/pack-metadata.yaml b/pack-metadata.yaml index 7cc4370..123cafb 100644 --- a/pack-metadata.yaml +++ b/pack-metadata.yaml @@ -17,6 +17,7 @@ scope: last_promoted_pr: 33 # last_promoted_at: YYYY-MM-DD demo_notes: >- - access.groups is enforced in the app pod by an authz sidecar (403 for - everyone else); a private app with no groups is "any signed-in user". + access.groups is enforced at the gateway by nebari-operator (403 for + everyone else; full group paths, so list subgroups); a private app with no + groups is "any signed-in user". Any authenticated user can manage every app. Launched apps are not on the landing page unless opted in. diff --git a/skill/new-nebari-app/SKILL.md b/skill/new-nebari-app/SKILL.md index 96bcdec..ac82451 100644 --- a/skill/new-nebari-app/SKILL.md +++ b/skill/new-nebari-app/SKILL.md @@ -14,7 +14,7 @@ and maps the manifest onto the `nebari-apps` MCP server's `launch_app` tool so - **Python app (pixi)**: a source tree with a `pixi.toml`; the platform runs `pixi install` then `pixi run ` (set in `runtime.pixiTask`), and the task must start a server on **0.0.0.0:$PORT** (`PORT` is injected: 8080, or 8081 when the app - lists allowed groups or can idle). + can idle). A third kind needs no scaffolding: an app that already ships as a **container image**. When `describe_cluster` lists `image` in `sourceTypes`, write a manifest with `source.type: image` diff --git a/skill/new-nebari-app/assets/python/pixi.toml b/skill/new-nebari-app/assets/python/pixi.toml index 95da1f8..a3bfdc1 100644 --- a/skill/new-nebari-app/assets/python/pixi.toml +++ b/skill/new-nebari-app/assets/python/pixi.toml @@ -9,6 +9,6 @@ fastapi = "*" uvicorn = "*" [tasks] -# The platform injects PORT (8080, or 8081 when access.groups is set or the app can idle) +# The platform injects PORT (8080, or 8081 when the app can idle) # and expects the task to serve on 0.0.0.0:$PORT. Locally: PORT=8080 pixi run start start = "uvicorn app:app --host 0.0.0.0 --port $PORT" diff --git a/skill/new-nebari-app/references/manifest.md b/skill/new-nebari-app/references/manifest.md index 61f5fdf..08f2a92 100644 --- a/skill/new-nebari-app/references/manifest.md +++ b/skill/new-nebari-app/references/manifest.md @@ -52,7 +52,7 @@ runtime: # optional # Python/pixi apps only: the pixi task that launches the app. When set, the # platform runs `pixi install` + `pixi run ` instead of serving the # source with nginx. The task must serve on 0.0.0.0:$PORT (PORT is injected: - # 8080, or 8081 when groups are set or the app can idle). + # 8080, or 8081 when the app can idle). pixiTask: start # Scale to zero after this many minutes without a request (5-43200); the next # visit wakes it. Omit for the cluster default (often off). @@ -63,7 +63,9 @@ runtime: # optional access: public: false # true = anonymous; false = Keycloak SSO at the gateway - groups: ["analytics"] # allowed groups (enforced in the app pod; 403 for others). + groups: ["analytics"] # allowed groups, full paths ("analytics" is "/analytics"; + # list subgroups as "/parent/child"). The gateway gives + # every other signed-in user 403. # Access is by group only: no per-user allow list managers: # optional: who else may manage (edit, stop, delete) the app; users: ["bob"] # not who may open it. Only the owner or an admin can change it @@ -141,7 +143,7 @@ content files and the manifest. copied into a writable workspace, `pixi install` resolves the environment (use a committed `pixi.lock` for reproducible, faster starts), and `pixi run ` launches the app. The task must start a server on **0.0.0.0:$PORT** — `PORT` is injected (8080, or -8081 when `access.groups` or idling puts the authz sidecar on 8080), so use +8081 when idling puts the authz sidecar on 8080), so use `--port $PORT` and run `PORT=8080 pixi run start` locally (pixi tasks do not support `${PORT:-8080}`). No Dockerfile needed. diff --git a/ui/src/components/app-form/sections.tsx b/ui/src/components/app-form/sections.tsx index c80de5c..8eed946 100644 --- a/ui/src/components/app-form/sections.tsx +++ b/ui/src/components/app-form/sections.tsx @@ -45,6 +45,7 @@ export const CONTROL_IDS: Record = { imageRunAsUser: "imageRunAsUser", idleTimeout: "idleTimeout", secrets: "secrets-first", + groups: "groups", subdomain: "subdomain", }; @@ -824,7 +825,7 @@ export function RuntimeSection({ form }: { form: FormContext }) {

Python apps must serve on 0.0.0.0:$PORT (8080, or - 8081 when groups restrict the app or it can idle). + 8081 when the app can idle).

@@ -869,13 +870,14 @@ export function AccessSection({ form }: { form: FormContext }) { set("groups", e.target.value)} /> @@ -1058,8 +1060,9 @@ export function ManagersSection({ form }: { form: FormContext }) { />

- Keycloak usernames and groups. Group membership is read from each person's sign-in token, - so a change to their groups applies when it refreshes. + Keycloak usernames and groups. Groups are full paths, as for allowed groups. Group + membership is read from each person's sign-in token, so a change to their groups applies + when it refreshes.

{editable ? null : (

!groupPath(g))) + add("groups", "Each group must name a Keycloak group, such as finance or /parent/child."); if (!isEdit) { if (!s.subdomain) add("subdomain", "Pick a subdomain."); else if (!SUBDOMAIN_RE.test(s.subdomain)) diff --git a/ui/src/lib/access.ts b/ui/src/lib/access.ts index 23d828d..a4659c9 100644 --- a/ui/src/lib/access.ts +++ b/ui/src/lib/access.ts @@ -1,8 +1,19 @@ import type { App } from "@/lib/types"; +/** + * The full Keycloak group path an access.groups entry names, the way + * nebari-operator matches it at the gateway: "finance" and "/finance/" are + * "/finance", and "" when the entry names no group (such as "/"). A parent + * path does not include its subgroups. + */ +export function groupPath(entry: string): string { + const segments = entry.trim().split("/").filter(Boolean); + return segments.length ? `/${segments.join("/")}` : ""; +} + /** * Who can open the app: "Public", any signed-in user, or the groups the - * authz sidecar lets in. + * gateway lets in. */ export function accessLine(a: App): string { if (a.access?.public) return "Public - no authentication"; diff --git a/ui/src/lib/crd.gen.ts b/ui/src/lib/crd.gen.ts index 0e6875a..479424a 100644 --- a/ui/src/lib/crd.gen.ts +++ b/ui/src/lib/crd.gen.ts @@ -3,11 +3,11 @@ // Do not edit: change operator/api/, regenerate the CRD, then run the script. export type AppAccess = { - /** Groups are the Keycloak/OIDC groups allowed to open a private app. */ + /** Groups are the Keycloak groups allowed to open a private app. */ groups?: string[]; /** Managers may manage the app (edit, start, stop, restart, delete, read logs and metrics) through apps-api, the UI and the MCP server, like its owner. */ managers?: { - /** Groups are Keycloak/OIDC groups whose members may manage the app (a leading "/" is ignored, as for api.adminGroups). */ + /** Groups are Keycloak groups whose members may manage the app, matched as full group paths like access.groups ("finance" is "/finance", which does not include "/finance/analysts"). */ groups?: string[]; /** Users are Keycloak usernames (preferred_username, case-insensitive). */ users?: string[]; diff --git a/ui/src/pages/settings.tsx b/ui/src/pages/settings.tsx index af96328..71504c1 100644 --- a/ui/src/pages/settings.tsx +++ b/ui/src/pages/settings.tsx @@ -281,9 +281,10 @@ function Account({ me }: { me: Query }) { description={ authEnabled ? ( <> - Your groups come from your sign-in. A restricted App opens for members of any group it - lists (and for users it names); you can manage the Apps you own and those whose managers - include you or one of your groups. Admins can manage every App. + Your groups come from your sign-in, as full Keycloak group paths. A restricted App opens + for members of any group it lists (a group does not include its subgroups); you can + manage the Apps you own and those whose managers include you or one of your groups. + Admins can manage every App. ) : ( <>Sign-in is off on this cluster, so everyone can see and manage every App. diff --git a/ui/tests/components/app-form-state.test.ts b/ui/tests/components/app-form-state.test.ts index 11639d3..4e195cf 100644 --- a/ui/tests/components/app-form-state.test.ts +++ b/ui/tests/components/app-form-state.test.ts @@ -8,7 +8,7 @@ import { splitList, validate, } from "@/components/app-form/state"; -import { accessLine } from "@/lib/access"; +import { accessLine, groupPath } from "@/lib/access"; import type { App } from "@/lib/types"; describe("AppForm reducer", () => { @@ -76,6 +76,36 @@ describe("validate", () => { }); }); +describe("allowed groups", () => { + const base = { + ...initialState(undefined, false), + nameInput: "docs", + displayName: "Docs", + namespace: "apps", + subdomain: "docs", + sourceTab: "git" as const, + gitUrl: "https://github.com/x/y", + isPublic: false, + }; + + it("matches nebari-operator's full-path form", () => { + expect(groupPath("finance")).toBe("/finance"); + expect(groupPath(" /finance/ ")).toBe("/finance"); + expect(groupPath("/ops//oncall")).toBe("/ops/oncall"); + expect(groupPath("/")).toBe(""); + expect(groupPath(" ")).toBe(""); + }); + + it("refuses an entry that names no group", () => { + expect(validate({ ...base, groups: "finance, /ops/oncall" }, false)).toEqual([]); + expect(validate({ ...base, groups: "finance, /" }, false).map((p) => p.field)).toEqual([ + "groups", + ]); + // A public app ignores its groups. + expect(validate({ ...base, groups: "/", isPublic: true }, false)).toEqual([]); + }); +}); + describe("idle settings", () => { const base = { ...initialState(undefined, false), diff --git a/ui/tests/components/app-form.test.tsx b/ui/tests/components/app-form.test.tsx index d5c6e09..ac3a2a2 100644 --- a/ui/tests/components/app-form.test.tsx +++ b/ui/tests/components/app-form.test.tsx @@ -124,7 +124,7 @@ describe("AppForm (create)", () => { const details = summary.closest("details") as HTMLDetailsElement; expect(details).not.toHaveAttribute("open"); expect(details).toHaveTextContent( - "Python apps must serve on 0.0.0.0:$PORT (8080, or 8081 when groups restrict the app or it can idle).", + "Python apps must serve on 0.0.0.0:$PORT (8080, or 8081 when the app can idle).", ); for (const label of [ "Replicas", @@ -196,7 +196,7 @@ describe("AppForm (create)", () => { it("describes who the groups field lets in", () => { setup(); expect(screen.getByLabelText("Allowed groups")).toHaveAccessibleDescription( - /Only members of these Keycloak groups can open the app; everyone else gets 403/, + /Only members of these Keycloak groups can open the app; the gateway gives every other signed-in user 403\. Groups are full paths: finance means \/finance, and \/finance does not include people who are only in \/finance\/analysts/, ); });