From 06e7938e767c3c3d3b32c457f782e25e3211e75c Mon Sep 17 00:00:00 2001 From: Alex Merced Date: Tue, 11 Aug 2026 18:40:27 -0400 Subject: [PATCH] docs: plan 0.9.0, and correct how the 0.8.0 image defects escaped CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds ROADMAP_0.9.0.md and documents the v0.8.0 tag/image discrepancy as a known, accepted issue rather than fixing it in place. Corrects a misdiagnosis I merged in the previous PR -------------------------------------------------- STATUS.md and CHANGELOG.md both said the four image defects escaped "because the `docker` job builds images and does not exercise what it built." That is wrong. The `docker` job builds the API image, starts it, probes its shutdown grace and fails if it runs as root — a genuine test. The actual gap is narrower and more actionable: it is the *only* image CI touches. `Dockerfile.tools` and `pangolin_ui/Dockerfile` are never built in CI at all, which is precisely why all four defects were in those two images and none in the API image. Fixing the wrong diagnosis would have produced the wrong remedy, so both files now say what actually happened, and R1 in the roadmap closes the real hole. The v0.8.0 tag/image drift -------------------------- `--version` was added to the CLIs after the tag was cut, so the released binaries reject `--version` while the container image accepts it. Impact is one help flag; no functional, data or security difference. Not corrected in place because re-cutting a published tag is the one thing a version number exists to prevent — and the release script already refuses the equivalent for images, so the tooling should not hold a stricter standard than the people using it. Resolved in 0.9.0 by R2, which also adds the guard that would have caught it. ROADMAP_0.9.0.md ---------------- Six buckets, ordered by what would hurt most if left undone: 1. Authorization — `authz.rs:39` returns Ok(true) for any TenantAdmin on any scope, and PermissionScope carries no tenant, so the check cannot be done without a lookup it never performs. Reachability across the 41 call sites is unverified. This is the most important item in 0.9.0. 2. Release integrity — CI must build and run all three images; the release must verify artefacts match the tag; publishing must move off a laptop. 3. Multi-replica — rate limiting and pending OAuth logins are both per-process. 4. Backend parity — 59 store-trait methods default to a runtime "not supported". 5. Debt with ratchets already in CI — clippy 30 to 0, and 0 of 90 UI components use Svelte 5 runes. 6. Operational hygiene — plaintext root password, the GitHub OIDC asymmetry, the GHSA decision. Every figure in the roadmap was confirmed against main with the command shown beside it, and each was re-run before commit: 41 call sites, 59 defaults, 0/90 components, 10 TODOs, both ratchet budgets, and all four named TODO lines. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 7 +- ROADMAP_0.9.0.md | 234 ++++++++++++++++++++ STATUS.md | 18 +- docs/known-issues/README.md | 2 + docs/known-issues/v0.8.0-tag-image-drift.md | 58 +++++ 5 files changed, 316 insertions(+), 3 deletions(-) create mode 100644 ROADMAP_0.9.0.md create mode 100644 docs/known-issues/v0.8.0-tag-image-drift.md 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.