diff --git a/CHANGELOG.md b/CHANGELOG.md index aa18c00..ce6bd51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,11 @@ operations, operational tooling, and OpenID Connect. ### Fixed — the published container images Four defects found by publishing 0.8.0 and then *running* what was published. -All 18 CI jobs passed over every one of them, because the `docker` job builds -images and does not exercise what it built. +All 18 CI jobs passed over every one of them — not because CI ignores images, +but because it only covers one. The `docker` job builds the API image, starts +it, probes its shutdown behaviour and fails if it runs as root. `Dockerfile.tools` +and `pangolin_ui/Dockerfile` are never built in CI at all, so the two images +with defects were the two nothing tested. - **`Dockerfile.tools` still pinned `rust:1.88`.** A-36 raised the API image to match the workspace MSRV and missed the CLI image, which then failed to diff --git a/ROADMAP_0.9.0.md b/ROADMAP_0.9.0.md new file mode 100644 index 0000000..ab08d43 --- /dev/null +++ b/ROADMAP_0.9.0.md @@ -0,0 +1,234 @@ +# Roadmap — 0.9.0 + +0.8.0 was about making the server correct and safe. 0.9.0 is about the two +things 0.8.0 exposed but did not finish: **authorization that is verified rather +than assumed**, and **a release process whose output is what the tag says it +is**. + +Every item below was confirmed against the code on `main` at the time of +writing, with the command that confirms it. Nothing here is recalled from +memory — this project's recurring failure is things that look done and are not, +so an item that cannot be demonstrated does not belong on the list. + +Ordered by what would hurt most if left undone. + +--- + +## Bucket 1 — Authorization correctness + +### A1. `TenantAdmin` bypasses the scope check entirely + +`pangolin_api/src/authz.rs:39` + +```rust +if session.role == RoleEnum::TenantAdmin { + // TODO: Check if scope matches session.tenant_id + return Ok(true); +} +``` + +Any `TenantAdmin` gets `true` for **any** scope, and `PermissionScope` carries no +tenant: `Catalog { catalog_id }`, `Namespace { catalog_id, namespace }`, +`Asset { catalog_id, namespace, asset_id }`. The function therefore *cannot* +verify the target belongs to the caller's tenant without a lookup it never does. + +Whether this is reachable depends on all 41 call sites independently enforcing +tenant scoping first. **That has not been verified, and this is the single most +important thing in 0.9.0.** In a multi-tenant catalog, a cross-tenant read is +the worst outcome the product has. + +- Audit all 41 `check_permission(` call sites for prior tenant enforcement. +- Resolve the scope's owning tenant inside `check_permission` and compare it to + `session.tenant_id`, so correctness does not depend on every caller. +- Add cross-tenant tests to the existing authorization matrix: tenant A's admin + must be denied on every scope variant belonging to tenant B. + +```bash +grep -rn "check_permission(" pangolin/pangolin_api/src/ | grep -v "fn check_permission" | wc -l +``` + +### A2. `Action::implies` and `Scope::covers` have no adversarial tests + +A1's fix rests on `grant.scope.covers(scope)`. The permission matrix tests +confirm allowed things are allowed; they do not systematically confirm that a +narrow grant fails to cover a broad scope. Add the negative direction. + +--- + +## Bucket 2 — Release integrity + +The 0.8.0 release published three images, four defects, and a tag that does not +match what shipped. Each item here closes one specific hole that actually opened. + +### R1. CI never builds two of the three images + +The `docker` job builds `pangolin-api:ci`, starts it, probes shutdown, and fails +if it runs as root. That is a genuine test — and it is the *only* image CI +touches. `Dockerfile.tools` and `pangolin_ui/Dockerfile` are never built in CI at +all, which is exactly why all four 0.8.0 defects were in those two images. + +Extend the `docker` job to build and exercise all three: + +| Image | Assertion | +|---|---| +| CLI | `pangolin-admin --version` and `--help` exit 0; runs non-root; no `/usr/include/openssl` | +| UI | serves `HTTP 200`; runs non-root; `node_modules` contains no build toolchain | + +The UI check is the one that matters most — `npm prune --omit=dev` builds clean +and fails at runtime if a runtime dependency is misfiled as a devDependency. + +### R2. Nothing verifies that published artefacts match the tag + +Cause of [the 0.8.0 tag/image drift](docs/known-issues/v0.8.0-tag-image-drift.md). +The release must refuse to publish when the working tree differs from the tag +being released, and record the source commit in an OCI label +(`org.opencontainers.image.revision`) so any published image can be traced back. + +Resolves the drift as a side effect: 0.9.0's tag and images will agree. + +### R3. Images are published from a laptop + +`scripts/build_docker_sequential.sh` runs by hand. The 0.8.0 run failed midway +and left the release half-published, and a stray `buildx` process survived a +`pkill` and had to be killed by PID before it pushed an image built from +pre-fix source. Move the push into the tag-triggered workflow that already +builds the binaries. + +### R4. `--locked` is not enforced everywhere + +Added to `Dockerfile.tools` in 0.8.0. `pangolin/Dockerfile` and the CI build +steps should match, so a published binary is always reproducible from the +committed `Cargo.lock`. + +--- + +## Bucket 3 — Multi-replica correctness + +Both of these are documented limitations rather than bugs. They become bugs the +moment someone scales past one replica, which the Helm chart makes easy. + +### M1. Rate limiting is per-process + +`pangolin_api/src/rate_limit.rs` uses an in-process `moka` cache, so N replicas +give an effective limit of N × the configured value. Documented in the upgrade +guide, but a brute-force limit that silently weakens with scale is the wrong +default. Needs shared backing, or a documented refusal to support it. + +### M2. OAuth requires session affinity + +The PKCE verifier is held in process — deliberately, since `state` travels +through the browser with the authorization code. Correct for security, but it +means an OAuth login breaks without sticky sessions. Same fix as M1: a shared +store for pending logins. + +--- + +## Bucket 4 — Backend parity + +### P1. 59 store-trait methods default to "Operation not supported" + +```bash +grep -c "Operation not supported by this store" pangolin/pangolin_store/src/lib.rs +``` + +Every one is a method some backend may not implement, failing at runtime rather +than compile time. This is how the cloud-credential features shipped without +ever compiling. Audit all 59: which are genuine "this backend cannot do that", +and which are unfinished work wearing the same error message. The parity suite +should assert the intended answer per backend. + +### P2. MongoDB branch-create-by-copy is not transactional + +PostgreSQL and SQLite got real transactions in 0.8.0; MongoDB falls back to +sequential statements and returns `500` naming the branch. Now that the +replica-set CI job exists, MongoDB transactions are testable. + +### P3. `commitTransaction` remains unimplemented + +Deliberate, and the only Iceberg REST endpoint still missing. Decide whether +0.9.0 implements it or documents it as permanently out of scope. + +--- + +## Bucket 5 — Debt with a ratchet already in place + +Both budgets are enforced in CI and both should move down in 0.9.0. + +| Ratchet | Now | Target | +|---|---|---| +| `pangolin/clippy-warning-budget.txt` | 30 (from 314) | 0 | +| `pangolin_ui/svelte-check-budget.txt` | 150 | materially lower | + +### D1. Svelte 5 runes: 0 of 90 components + +```bash +grep -rl '\$state\|\$derived\|\$props' pangolin_ui/src --include='*.svelte' | wc -l # 0 +find pangolin_ui/src -name '*.svelte' | wc -l # 90 +``` + +The UI runs on Svelte 5 in legacy compatibility mode. It works, and it is a +deprecation clock. 0.8.0 called this migration complete — that was accurate for +*compiling and running* under Svelte 5, but no component uses the new reactivity +model. Migrate in tranches with the `svelte-check` budget ratcheting down. + +### D2. Ten remaining `TODO`/`FIXME` markers + +```bash +grep -rn "TODO\|FIXME" --include="*.rs" --include="*.svelte" pangolin/ pangolin_ui/src | grep -v /target/ | wc -l +``` + +A1 is one of them. Triage the rest: fix, convert to a tracked item, or delete. +Notable: `user_handlers.rs:681` (token invalidation), `merge_handlers.rs:281` +(`merge_branch` does not return a commit ID), `oauth_handlers.rs:635` (config +loaded from neither environment nor file). + +--- + +## Bucket 6 — Operational hygiene + +### O1. `.env` holds `PANGOLIN_ROOT_PASSWORD` in plaintext + +Compose passes it into containers. Flagged during the 0.8.0 audit and not acted +on, because changing it is a decision about how operators are expected to run +the thing, not a bug fix. 0.9.0 should decide: secret file, external secret +manager, or an explicit documented statement that this is the supported way. + +### O2. GitHub OAuth cannot be OIDC-validated + +GitHub issues no `id_token` and publishes no JWKS, so its logins rest on the +userinfo endpoint while every other provider gets full validation. +`PANGOLIN_OIDC_REQUIRE=true` refuses it. This is a permanent property of the +provider — the work is making the asymmetry obvious in the UI, not just the +docs. + +### O3. Publish the GHSA + +Requires a person. A fixed version now exists on all three channels, which was +the precondition. + +--- + +## Explicitly not in 0.9.0 + +- **Re-cutting `v0.8.0`.** Replacing a published tag is the one thing a version + number exists to prevent. See the known-issue document. +- **1.0.0.** Not until A1 is resolved and the parity audit in P1 is finished. + A multi-tenant catalog should not call itself 1.0 with an unverified + cross-tenant authorization path. + +--- + +## How to judge 0.9.0 complete + +The standard that caught everything worth catching in 0.8.0: **an item is done +when something executes it, not when something reads it.** + +Seven times in 0.8.0 a change compiled, passed all 18 CI jobs, and did not work +— cloud-credential features that had never compiled, a cleanup job that never +started, a release pipeline that never released, two guardrail jobs that never +ran a test, a server that killed itself 25 seconds after boot, a load harness +that understated latency by 1000×, and four defects in two container images that +CI never built. Every one was found by running the artefact. + +So: no item above is complete on the strength of a green tick alone. R1 exists +precisely because a green tick was not enough. diff --git a/STATUS.md b/STATUS.md index 6d1d87b..e4618b4 100644 --- a/STATUS.md +++ b/STATUS.md @@ -138,7 +138,11 @@ the build's exit code: the CLI reports `pangolin-admin 0.8.0` and runs as uid 10001, the UI serves `HTTP 200` as uid 1000 with a 2.3MB `node_modules`. Publishing them turned up four defects that every one of the 18 CI jobs had -passed over, because CI builds images and never runs what it built: +passed over. The reason is narrower than "CI does not test images": the +`docker` job builds the **API** image, starts it, probes its shutdown grace and +fails if it runs as root. It is a real test. But it is the only image CI touches +— `Dockerfile.tools` and `pangolin_ui/Dockerfile` are never built in CI at all, +so nothing exercised them until a human ran the release script: | Defect | Consequence | |---|---| @@ -164,6 +168,18 @@ predates every security fix listed above. Still requiring a person: decide whether to publish a GHSA now that a fixed version exists. +## Next + +[ROADMAP_0.9.0.md](ROADMAP_0.9.0.md) lists the outstanding work, ordered by what +would hurt most if left undone. The top item is an unverified cross-tenant +authorization path (`authz.rs:39`), and the second is that CI never builds two of +the three container images — the reason all four 0.8.0 image defects survived a +green pipeline. + +One known discrepancy is accepted and documented rather than fixed: +[the `v0.8.0` tag and the `0.8.0` image differ](docs/known-issues/v0.8.0-tag-image-drift.md) +by a single CLI help flag. + ## If you are deciding whether to run this The honest summary: the security holes found in the audits are fixed and there diff --git a/docs/known-issues/README.md b/docs/known-issues/README.md index 8f65e5e..3f4a7f0 100644 --- a/docs/known-issues/README.md +++ b/docs/known-issues/README.md @@ -13,3 +13,5 @@ This section documents verified issues, limitations, and architectural quirks pr No verified issues are open against 0.8.0 beyond the limitations recorded in [STATUS.md](../../STATUS.md), which is the authoritative list. + +- [The `v0.8.0` tag and the `0.8.0` container image differ](v0.8.0-tag-image-drift.md) — `--version` is missing from the released CLI binaries but present in the image. Cosmetic; resolved in 0.9.0. diff --git a/docs/known-issues/v0.8.0-tag-image-drift.md b/docs/known-issues/v0.8.0-tag-image-drift.md new file mode 100644 index 0000000..e3acba9 --- /dev/null +++ b/docs/known-issues/v0.8.0-tag-image-drift.md @@ -0,0 +1,58 @@ +# The `v0.8.0` tag and the `0.8.0` container image differ + +**Status:** known, accepted, scheduled for 0.9.0 +**Affects:** `pangolin-admin` and `pangolin-user` only +**Impact:** cosmetic — a missing `--version` flag. No functional, data or +security difference. + +## What differs + +`--version` was added to both CLIs *after* the `v0.8.0` tag was cut, and the +container images were then built from that newer commit. So one version number +describes two slightly different builds: + +| Artifact | `pangolin-admin --version` | +|---|---| +| GitHub release `v0.8.0` binaries | `error: unexpected argument '--version' found` | +| `alexmerced/pangolin-cli:0.8.0` | `pangolin-admin 0.8.0` | + +Verifiable directly: `command(version)` appears zero times at `git show +v0.8.0:pangolin/pangolin_cli_admin/src/main.rs` and once on `main`. + +Everything else — the server, the SDK, the UI, the API image, every security +fix in 0.8.0 — is identical across both. The divergence is one clap attribute +on two CLI binaries. + +## Why it happened + +Publishing 0.8.0 failed partway: the CLI image would not compile because +`Dockerfile.tools` still pinned `rust:1.88`. Fixing that meant running the CLI +image for the first time, which surfaced three further defects, one of them the +missing `--version`. Those were fixed and the images rebuilt — but the tag had +already been cut and the GitHub release binaries already published. + +The mistake was sequencing, not the fixes: a defect found after tagging should +either be held for the next version, or the tag should not yet exist. + +## Why it was not corrected in place + +Re-cutting `v0.8.0` would replace a published tag with different content, which +is the single thing a version number exists to prevent. The release script +already refuses the equivalent for container images +(`scripts/build_docker_sequential.sh` will not overwrite a published tag without +`ALLOW_OVERWRITE=1`), and the tooling should not hold a stricter standard than +the humans using it. + +Given the impact is a missing help flag, spending a `0.8.1` release on it was +judged not worth it either. + +## Resolution + +0.9.0 makes the two consistent, and adds the guard that would have caught it: +the release must verify that the artefacts it publishes were built from the tag +it is publishing. See [the 0.9.0 roadmap](../../ROADMAP_0.9.0.md), item R2. + +## If you are affected + +Use the container image if you need `--version`, or read the tag you pulled. +`pangolin-admin --help` works identically in both.