nordisk/holger

Security review — holger artifact server (2026-09-11)

Read-only review of main at 4ae7021, under holger's own threat model: an airgap-first artifact registry whose promise is that reads may be open on the secure side but writes, promotion and admin are gated, storage is immutable and content-addressed, and everything is audited. Successor to docs/security-review-2026-06-14.md; every finding that review raised is re-checked here and its status recorded.

Method. Fourteen dimensions of the attack surface were swept independently, and every finding was then handed to three refuters instructed to break it — one reading for strict correctness, one for reachability, one for impact. A finding is listed below only if at least two of the three failed to refute it. Nineteen claims did not survive and are not reported as defects; six low-value observations went unverified and are listed as such. Counts: 70 candidate findings verified, 51 survived, deduplicated to 40 distinct defects.

What could not be done. The workspace does not build on the review machine (edda main floors jera at draupnir 0.1.11, which exists in no draupnir checkout or branch here), so nothing was executed: no test run, no fuzzing, no dynamic proof. Every claim below is from reading the code, and every one cites the lines it rests on. cargo audit needs only the lockfile and did run; cargo deny needs the same resolution the build does and did not.

Executive summary

The structural problems the June review found are largely fixed. Path validation now exists and is enforced (C-1, H-1), authorization is real and per-repo (H-4), the audit log is wired at both doors (H-7), and error messages no longer leak server paths (M-3). That is genuine progress on the things that made the product pitch hollow.

What remains falls into four groups, and only the first is a surprise.

1. The per-repo Denied carve-out does not cover the cross-repo doors. Search and repository listing authorize the empty repository name — the global floor — and then read every repository. An identity explicitly denied a repository can still enumerate its coordinates, versions and archive paths, on both the gRPC and the HTTP door, and this holds in the Critical profile where the read gate is mandatory. Replication and promotion have the same shape from the other side: they authorize the destination repository and read the source with no gate at all. Denied is documented as "nothing — not even reads"; on these four paths it is not that.

2. Reads are an unauthenticated amplifier. Several read paths convert a small request into a large amount of server work: /-/search with a checksum axis reads and SHA-256s every artifact in every repository; the Deb index memoizes under a caller-chosen key so every request re-decompresses the whole pool; Gem and Conda and the Rust metadata endpoints have the same shape; a malicious Helm chart is gzip-decompressed with no output cap; and both gRPC fetch paths hold a whole artifact in memory with no concurrency bound. None of these needs a credential in the default posture.

3. A configured OIDC deployment silently stops checking audience. This is the one finding that changes a security property rather than an availability one, and it is examined in full below. In short: with the configuration every shipped guide teaches, the JWKS never resolves, the offline JWT path fails, and the provider falls back to /userinfo — which authenticates the token but checks neither issuer nor audience. A token minted for any other application at the same IdP then authenticates to holger.

4. The dependency surface has gone from clean to 29 advisories, unwatched. In June cargo audit reported zero vulnerabilities. It now reports 29 plus 18 warnings. Nothing in the repository runs cargo audit or cargo deny, there is no deny.toml, and there is no CI at all — .github/ contains only copilot-instructions.md. Nothing would have told anyone.

No arbitrary file write, no arbitrary file read, no authentication bypass and no secret leak was found. The two Critical findings from June are not reachable in the same form.

Status of the June 2026 findings

Nine of twenty closed, one regressed. The closures are the structural ones.

High

H-1 · Denied does not cover enumeration: Search and ListRepositories authorize the empty repository name

server/lib/src/grpc/services.rs:1040, server/lib/src/exposed/http.rs:326

With require_auth_for_reads: true (mandatory under the Critical profile, server/lib/src/lib.rs:1494-1498) and repo_roles: {"classified": {"carol": Denied}}, Carol's direct read of classified is correctly refused. Her Search with an all-empty query is not: the rpc authorizes "", which resolves to her global role, and then searches all_repos(). The HTTP twin does the same at http.rs:326, where the gate is called with a hard-coded "" scope. She gets coordinates, versions, archive paths and a content-presence oracle for every repository she is explicitly denied.

Fix. Filter rather than gate once: authenticate, then build the repository list from all_repos() filtered by authorize_read(identity, repo).

H-2 · A malicious Helm chart OOM-kills the server on the first index.yaml request

server/repository/znippy-helm/src/index.rs:48

Index synthesis calls the uncapped gzip entry point on every chart tarball in the repository and holds them all in memory at once. A ~10 MB chart crafted near DEFLATE's 1032:1 ratio expands toward 10 GB. The trigger is an unauthenticated GET /{repo}/index.yaml, and because the result is memoized in a OnceLock the process never gets past it.

Fix. Use the capped decompression variant with a limit in the low tens of MiB; a Chart.yaml is a few KiB.

H-3 · /-/search with a checksum axis reads and hashes every artifact on the server

server/lib/src/exposed/http.rs:330, server/lib/src/search.rs:255

One unauthenticated GET /-/search?checksum=<64 hex> passes every repository into the search engine, which reads and SHA-256s every artifact to compare. limit is never clamped (http.rs:1001-1005) and is passed to the backend as a hard-coded 0. A forty-byte request becomes a full-store read-and-hash.

Fix. Clamp limit at parse time and pass it down; refuse or gate the checksum axis on the unauthenticated door.

H-4 · FetchArtifact and StreamArtifact hold whole artifacts in memory, with no concurrency cap

server/lib/src/grpc/services.rs:427 and :147-159

StreamArtifact loads the entire blob before chunking, so the chunking buys no memory bound — the June H-6 shape, unchanged. Reads are open by default (grpc.rs:438), there is no in-flight-bytes limit and no concurrency_limit_per_connection. One HTTP/2 connection's 200 streams against a 200 MB layer asks for roughly 40 GB.

Fix. Serve from a streaming backend API rather than Vec<u8>, and add a global in-flight-bytes semaphore plus a tonic concurrency limit and request timeout.

H-5 · The Deb index memoizes under a caller-chosen key

server/repository/znippy-deb/src/lib.rs:279

GET /{repo}/dists/<anything>/Release is cached under the request string, so any new path misses and re-runs a full decompress, SHA-256 and control-parse pass over every package in the pool. The attacker varies one path segment.

Fix. Key the cache on the derived identity of the work, not on the request string, and memoize the inner unit.

Medium

The seventeen medium findings, with the line each rests on. Those marked ▲ change a security property; the rest are availability or integrity-of-service.

Low

Eighteen confirmed low findings, in brief. Each was verified the same way.

• group_roles is parsed, flips RBAC on, and is consulted by no gate — a group-scoped Denied or demotion silently does not apply (nornir-auth-policy/src/lib.rs:233; the gates use role_for_repo, server/lib/src/grpc.rs:378, server/lib/src/exposed/http.rs:1231). Found independently by four dimensions. Dead policy that reads as live policy.
• /metrics ignores require_auth_for_reads and serves the repository catalogue with artifact counts to unauthenticated callers, with an unbounded listing per scrape (server/lib/src/exposed/http.rs:488).
• Every boot-time SECURITY: warning is below the default log level of the shipped binary and the systemd unit, so an operator cannot tell from the output that a control is off (server/cli/src/main.rs:1126).
• A mistyped RON key is silently ignored, so a config that reads as Critical can boot as Standard with nothing said (server/lib/src/lib.rs:487).
• The Critical profile's "a static bearer must not be the only mechanism" check is satisfied by an auth method the build cannot serve, and the shipped holger-server links no OIDC at all (server/lib/src/lib.rs:1500).
• The update-bundle verifier never binds core.json's declared payload and evidence digests to the actual members, so approvals cover a content list nobody checks (update-bundle/src/verify.rs:296).
• A decompression bomb exhausts memory before any signature is checked, and the kill leaves the import lock wedged (update-bundle/src/container.rs:108).
• Backup archives have no authenticity layer: backup verify and backup restore trust a hash list carried inside the archive being checked (server/cli/src/secure_update.rs:486).
• The sparse-index forward passes the client's raw path and query to the upstream origin, .. included (server/lib/src/upstream_rust.rs:136).
• The Cargo sparse index is hand-built without JSON escaping, so a publisher-controlled version string injects index entries (server/repository/znippy-rust/src/lib.rs:493).
• The Maven typed-view read path sizes allocations from the untrusted archive index without the bounds check its sibling has (server/repository/znippy-maven/src/lib.rs:136).
• The HTTP accept loop has no connection cap, no TLS-handshake timeout, and spins hot when accept() fails (server/lib/src/exposed/http.rs:160).
• ReadOnly operation mode does not cover the marketplace callback, which provisions a tenant over GET (server/lib/src/exposed/http.rs:297).
• The rate limiter reads only the first X-Forwarded-For header, so behind a proxy that appends a second header line the bucket is caller-chosen (server/lib/src/exposed/http.rs:564).
• The marketplace landing door parks an uncapped purchase token in the in-memory pending map for ten minutes; the 64 KB body cap does not apply to a query string (server/lib/src/marketplace/door.rs:41).
• The purchase token is marked spent before the buyer-directory check, so a stranger's failed attempt consumes the real buyer's single use (server/lib/src/marketplace/mod.rs:951).
• pip airgap ingestion fails open from pinned wheels-only to unpinned sdist execution (distribution/airgap/airflow/build-and-push.sh:33).
• The k8s manifests still lack a seccomp profile and a NetworkPolicy, and the NodePort service is still present (deploy/k8s/).

Six further observations were reported without verification and are recorded for completeness, not as findings: WhoAmI as an ungated credential-validation oracle; the Docker manifest Content-Type echoed from the manifest's own mediaType; the bundle-import audit record written after the import commits; OCI upload session ids as a process counter; the bundle decompression guard allocating before it checks the cap; and the wasm plugin host running guest code with no time or memory bound.

Dependencies

cargo audit on Cargo.lock: 29 vulnerabilities, 18 warnings, against zero in June. Full output was captured during the review.

• 17 are wasmtime, including four sandbox escapes and several host panics. wasmtime enters the graph only through the wasm-plugins feature, which is off in every shipped tier — the claim is in server/lib/Cargo.toml and holds on reading. They are therefore not reachable in a shipped build, but they are reachable for anyone who turns the feature on, which is the feature's whole point.
• rsa carries RUSTSEC-2023-0071 (Marvin timing side-channel) with no fixed version available upstream.
• h2, quick-xml and quinn-proto carry remote memory-exhaustion or quadratic-parse advisories and sit under reachable paths.
• Three crates in the graph are yanked (bisync, chacha20, spin).

The process gap matters more than any single advisory: nothing watches this. There is no CI (.github/ holds only copilot-instructions.md), nothing in the tree invokes cargo audit or cargo deny, and there is no deny.toml. The release procedure in CLAUDE.md is entirely manual and does not mention either tool. Three months of drift went unremarked.

No secret is committed. deploy/k8s/secret.yaml is a placeholder template that recommends cert-manager or SOPS; the private-key string in server/cli/src/main.rs is a test fixture. Historical test certificates from the removed holger-mannequin/ and holger-ron/ paths remain in git history; mannequin now generates its CA in memory, so nothing in the tree uses them.

What to fix first

Ranked by risk reduced per unit of work.

1. Filter the cross-repo doors by role instead of gating once on "". Closes H-1 and M-2 together, and restores the meaning of Denied. Four call sites.
2. Make an OIDC method with no resolvable JWKS a boot error, and correct the default jwks_uri or remove the default entirely. Closes M-1 — the one finding that lets a token from elsewhere authenticate — and finishes June's C-2. One condition at boot plus a documentation pass over the five guides that teach the incomplete form.
3. Bound the read paths. Clamp limit and pass it down; cap the Helm decompression; key the Deb and Conda caches on derived identity; add a concurrency and in-flight-bytes limit to the gRPC serve tail. Closes H-2, H-3, H-5 and much of H-4.
4. Wire cargo audit and cargo deny into something that runs, with a deny.toml, and record the wasmtime advisories as accepted-while-the-feature-is-off rather than leaving them unexplained. The gap that let 0 become 29 is the absence of a watcher, not any one crate.
5. Give the quarantine gate and the reserved property keys a floor. Make serve_quarantined fail closed when it cannot map a path to a coordinate, and require admin for the keys the gates trust. Closes M-3 and M-4.

Method note

Fourteen dimensions were swept: the HTTP door, the marketplace seam, the gRPC surface, the authorization model, persistence, the Rust/JVM/Python backends, the OS and OCI backends, the proxy and upstreams, the update bundle, crypto and secrets, DoS and resource bounds, supply chain, configuration and fail-closed behaviour, and the airgap and appliance lane. 224 agents, 70 candidate findings, three refuters each on everything above low severity.

Nineteen candidate findings were refuted and are deliberately absent. They included several that would have read as serious: that a per-repo Denied is a no-op for reads; that property removal is reachable with the Writer role; that the marketplace doors serve over cleartext; that random_hex fails open to a clock-derived value; that the ISO installer enrols a published development seed as the fleet root of trust; and that the release container ignores the audited lockfile. Each was checked against the code and did not hold. They are listed here because a reader who later finds one of them independently should know it was examined and dismissed, not missed.

Fix status (2026-09-11, branch fix/security-review)

All 40 distinct defects are closed. Every fix carries a test that was proven RED: the production change was reverted, the test was run and observed to fail, and only then was the change restored. A test that passes with and without its fix proves nothing and none were accepted.

The workspace also builds and runs now, which the review itself could not do. The constellation gap it describes was worked around with a scratchpad worktree beside symlinked siblings, so the claims below are executed, not read: 973 tests pass across 76 targets, 0 failures (excluding holger-mannequin and holger-agent-cli, which do not compile for a pre-existing reason unrelated to this work — facett's egui 0.34 against the workspace's 0.35 — and bench-scenarios, whose art_probe and retention_probe examples are missing required-features declarations, also pre-existing).

Where the fixes changed the shape of the product

Six fixes do more than close a hole, and an operator will notice them:

• A boot refusal for an OIDC method with no resolvable key material. The documented config silently degraded to /userinfo, which checks neither issuer nor audience. A deployment that relied on that now refuses to start, with a message naming the issuer, the hazard and the one-line escape hatch (auth_allow_unbound_oidc: true). This is the single most disruptive change here and it is deliberate: the degrade was invisible, so the fix must not be.

A method whose only key source is jwks_uri is treated separately, and boots with a loud warning instead. That hazard is conditional rather than certain — a correct, reachable endpoint does verify and does pin issuer and audience, and it degrades only when the fetch yields nothing — and refusing it would refuse holger's own marketplace tenants, every one of which is provisioned with exactly that shape. A control that refuses the product's primary configuration is a control that gets switched off.

• A boot refusal for a mistyped RON key. Unknown keys used to be ignored, so a config reading as Critical could boot as Standard. Config is now read strictly. The corollary rule is recorded at read_ron_config: retire a field by keeping the key and ignoring it, never by deleting it.

• A boot refusal when the quarantine gate cannot be enforced. Closing the serve-time gate's fail-open left two honest behaviours for a backend that cannot map a served path back to a coordinate: serve its artifacts ungated, or refuse all of them. Both are unacceptable as a runtime surprise, so the choice moved to boot and the refusal names the repositories. Only raw declares the mapping today, so on a server with quarantine configured this is a real constraint, and it is scoped to the surface that has the problem: the gate exists only on the HTTP door, so a repository wired to a gRPC-only endpoint is unaffected whatever its path scheme. That scoping is what makes the remedy the message offers ("serve those repositories on a gRPC-only endpoint") actually work.

• retention --protect-property refuses a key the server does not reserve. The pin lives in a CLI flag and the gate that stops a Writer setting that same property lives in the server; they were never connected. A pin any Writer can set is not a pin, so the two halves now read one resolution (Holger::reserved_property_keys) and disagreement is a refusal.

• Directory groups now reach the gates — reads included. group_roles was parsed, counted towards rbac_enabled() (switching RBAC on for the whole server), and then consulted by no gate. An operator's group grant was silently dropped on a server their own config had just made strict. Every write, admin and read gate now resolves through the shared leaf's group-aware resolver. The read half is the one that nearly got away: migrating only writes and admin would have refused a group granted Reader every read while the same group's Writer grant worked for uploads, which is worse than the original defect.

A group can only GRANT, never deny — group roles combine by taking the highest — so a group_roles entry set to Denied reads as a denial and is not one. It is shouted about at boot rather than silently honoured as nothing.

• Metadata writes refuse an aliased repository name. Property and SBOM writes authorized the caller's raw string and stored under sanitize_repo of it, a many-to-one map. rust/prod matched no per-repo ACL rule, took the permissive default, and landed in the real rust_prod — a per-repository ACL bypass and a quarantine-flag bypass in one move. Such a name is now refused, never redirected, and the stores are closed to the repositories the server actually serves.

The fixes were then reviewed the same way

Before merging, the commit itself was swept by six independent lenses — does each fix close its hole, what legitimate deployment does it break, did parallel workers contradict each other, do the tests assert what they claim, is the prose true, what breaks for consumers — and every candidate finding was put to three refuters with distinct angles, surviving only on a majority. Twenty-four candidates, five survived, all five fixed here:

• The reserved-property ADMIN floor was on one door of two. The gRPC SetProperty RPC gained it; the HTTP /-/properties door writes the same store through the same PropertyStore::set and had only the plain Writer gate. A Writer refused at one door simply used the other — setting the keep retention pin, forging the signed=true the promote gate trusts, or clearing a quarantine flag. keep is reserved by default, so this needed no special configuration. Both doors now ask one shared rule, and a door-level test proves the wiring rather than the rule.
• Directory groups reached the write gates but not the read gate. Half a fix, and the worse half: a group granted Reader would be refused every read while the same group's Writer grant worked for uploads.
• The quarantine boot check asked the wrong backend. It inspected the unwrapped backend while the door serves through a ProxyBackend or GroupBackend, neither of which forwarded the capability — so a proxied raw repository passed the check and would then have refused every artifact at the door. Both wrappers now forward it, the group fail-closed across its members.
• The same check was also too broad, refusing over repositories served on a gRPC-only endpoint where the gate it guards cannot fire — while its own message offered exactly that as the remedy.
• role_for_repo_identity was a twin. The shared leaf already ships role_for_identity_in_repo with identical precedence. Deleted; every gate calls the leaf.

Deliberately not done

• The four deferrable advisories are now closed (2026-09-12). h2 (RUSTSEC-2026-0258, remote memory exhaustion, under every HTTP/2 door), quinn-proto, crossbeam-epoch and webbrowser were bumped by a targeted cargo update, and their ids are gone from deny.toml and the audit workflow. Getting there needed the constellation repaired first: two sibling repositories (draupnir, modgunn) had lost their git remote while the local checkouts sat behind the versions the lockfile required, and the published crates were the only surviving copy of the missing work.

• The remaining 20 advisories are documented, not silenced. Seventeen are wasmtime, reachable only if wasm-plugins is turned on, and that acceptance expires the moment it is; one is rsa with no upstream fix and no decryption oracle in holger's use; two are quick-xml and are marked TRACKED, NOT ACCEPTED — they are reachable through maven pom parsing and the fix belongs in znippy.

• Nothing was changed in edda. Two auth residues live in the shared provider there, and holger's own boot gates close them for holger only — it refuses the configuration that reaches the first, and shouts at every boot about the one it cannot refuse without refusing its own marketplace tenants. Both are raised with that repository rather than described here, because they are unfixed and the shared crate has consumers beyond holger. The holger-side detail is in server/lib/src/auth.rs, which is where anyone picking this up should start.

• deny_unknown_fields stops at the top level. It is on Holger and AuditSettings; it is not on Repository, ExposedEndpoint, StorageEndpoint, or on AuthConfig, which lives in the shared leaf and is not holger's to annotate. A typo inside the auth block is still dropped in silence, which is the worst place left for one. holger keeping its own copy of the leaf's field names would drift in the fail-open direction, so the gap is pinned by a test instead and the fix raised upstream. The boot error's wording was narrowed to claim only the strictness that exists.

• Four documents still carry the vulnerable OIDC example (.nornir/auth-guide.md, auth-rbac-design.md, auth-design.md, auth-for-idiots.md). Every deployment copied from them will now fail to boot with a message naming the fix, but the documents should be corrected before this ships.

• FetchArtifact still holds a whole artifact in memory. The in-flight byte budget and the new process-wide concurrent-read cap bound the damage, and a request timeout belongs in the shared serve tail in edda. The streaming backend API the review asks for is a change to RepositoryBackendTrait and every implementation of it.