diff --git a/internal/build/build.go b/internal/build/build.go index 02ce21d..b1bae1a 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -78,10 +78,28 @@ const ( JobFailed ) -// image admission sources (spec §6 image_whitelist.source). +// image admission sources (spec §6 image_whitelist.source). The column is plain +// text, not an enum, so a curated value costs no schema change. const ( SourceBuilt = "built" SourceExternal = "external" + // SourceRecommended marks a platform-curated whitelist entry: an image Felis + // itself ships and vouches for, which the create-server form may surface ahead + // of the rest. It is a PRESENTATION marker only — admission still turns solely + // on enabled (see ImageAdmitted), so a recommended row is admitted by exactly + // the same rule as any other and carries no extra privilege. + // + // The bar for this marker is joinability, not popularity. Velocity runs + // proxy-wide modern forwarding, so a backend that cannot verify the signed + // handshake rejects every login the proxy sends it; only an image whose + // entrypoint consumes FELIS_FORWARDING_SECRET is actually reachable by a + // player (see operator.buildEnv, which injects it into every backend but + // cannot make an operator-typed Dockerfile read it). Recommending an arbitrary + // public Minecraft image would therefore ship a trap: it builds, schedules, + // and goes Ready, then refuses every join. Only the images Felis builds from + // deploy/ clear that bar — see 0018_recommended_images.sql for which, and why + // the honest set is one image rather than several. + SourceRecommended = "recommended" ) // ErrNotFound is returned when a build id / image ref does not exist. diff --git a/internal/build/build_test.go b/internal/build/build_test.go index 6fe2a14..1d7e1a0 100644 --- a/internal/build/build_test.go +++ b/internal/build/build_test.go @@ -529,3 +529,35 @@ func TestImageAdmitted(t *testing.T) { } } } + +// A 'recommended' row (0018) is curation, not capability: it must be admitted by +// exactly the rule that governs every other source, and it must not become a way +// to bypass the disable switch. Both halves are pinned here because the failure +// modes are silent and opposite — make admission source-aware in one direction +// and the curated images quietly vanish from the create-server form; in the +// other, a disabled recommendation stays creatable after an admin pulled it. +func TestRecommendedImageAdmittedLikeAnyOtherSource(t *testing.T) { + b, st, _ := newBuilder() + st.images["felis-lobby:demo"] = Image{ + ImageRef: "felis-lobby:demo", Source: SourceRecommended, Enabled: true, + } + st.images["felis-lobby:pulled"] = Image{ + ImageRef: "felis-lobby:pulled", Source: SourceRecommended, Enabled: false, + } + + admitted, err := b.ImageAdmitted(context.Background(), "felis-lobby:demo") + if err != nil { + t.Fatalf("ImageAdmitted: %v", err) + } + if !admitted { + t.Error("an enabled recommended image must be admitted; curation must not cost admission") + } + + admitted, err = b.ImageAdmitted(context.Background(), "felis-lobby:pulled") + if err != nil { + t.Fatalf("ImageAdmitted: %v", err) + } + if admitted { + t.Error("a disabled recommended image must not be admitted; curation is not a disable bypass") + } +} diff --git a/internal/build/pgstore.go b/internal/build/pgstore.go index 8a86110..32a7c1b 100644 --- a/internal/build/pgstore.go +++ b/internal/build/pgstore.go @@ -128,12 +128,24 @@ func (s *PGStore) ListUnfinishedBuilds(ctx context.Context) ([]Build, error) { // AdmitBuiltImage upserts the whitelist row on scan-gate success. ON CONFLICT // re-enables and re-stamps a previously-removed or superseded ref, so a rebuild // of the same tag re-admits it (spec §16). +// +// source is the ONE column the conflict path does not overwrite unconditionally: +// a 'recommended' row is platform curation (0018), while every other field here +// describes the build that just succeeded and must win. Rebuilding a curated tag +// is the expected way to patch it, and that rebuild arrives through this exact +// path — so a blind `SET source = 'built'` would silently demote the curation on +// the first rebuild, with nothing in the request saying so. Preserving it keeps +// the marker a deliberate admin decision: DELETE /images is the way to clear it, +// not a build completing. Only 'recommended' is sticky; an 'external' row still +// becomes 'built', because a real build genuinely supersedes a hand-pushed ref. func (s *PGStore) AdmitBuiltImage(ctx context.Context, img Image) error { const q = `INSERT INTO image_whitelist (image_ref, source, build_id, added_by, enabled, added_at) VALUES ($1, 'built', NULLIF($2, ''), $3, true, $4) ON CONFLICT (image_ref) DO UPDATE - SET source = 'built', build_id = EXCLUDED.build_id, added_by = EXCLUDED.added_by, + SET source = CASE WHEN image_whitelist.source = 'recommended' + THEN 'recommended' ELSE 'built' END, + build_id = EXCLUDED.build_id, added_by = EXCLUDED.added_by, enabled = true, added_at = EXCLUDED.added_at` _, err := s.db.ExecContext(ctx, q, img.ImageRef, img.BuildID, img.AddedBy, img.AddedAt) return err @@ -163,6 +175,19 @@ func (s *PGStore) ListImages(ctx context.Context) ([]Image, error) { return out, rows.Err() } +// AddExternalImage upserts a hand-pushed ref. Unlike AdmitBuiltImage this path +// does NOT preserve a 'recommended' source, and the asymmetry is deliberate: an +// admin POSTing this exact ref is an explicit, named re-admission, not a build +// completing behind their back, so demoting the curated row is the outcome they +// asked for. It also keeps the 201 body honest — AddExternalImage returns the +// Image it constructed (source=external) without re-reading the row, so a sticky +// source here would report a value the database does not hold. +// +// Note that the demote branch is unreachable for the ONLY recommended row Felis +// currently seeds: the caller validates with ValidateImageRef first, which refuses +// a bare local containerd tag, and 0018's felis-lobby:demo is exactly that. The +// branch is written for the host-qualified recommendations this list grows into, +// not for today's single seed. func (s *PGStore) AddExternalImage(ctx context.Context, img Image) error { const q = `INSERT INTO image_whitelist (image_ref, source, added_by, enabled, added_at) diff --git a/internal/store/migrations/0018_recommended_images.sql b/internal/store/migrations/0018_recommended_images.sql new file mode 100644 index 0000000..25ede04 --- /dev/null +++ b/internal/store/migrations/0018_recommended_images.sql @@ -0,0 +1,62 @@ +-- Recommended images: platform-curated entries the create-server form can offer +-- ahead of the rest of the whitelist. image_whitelist.source is plain text, not +-- an enum, so this needs no schema change — 'recommended' simply joins 'built' +-- and 'external' as a third provenance (see internal/build.SourceRecommended). +-- The marker is presentation only: admission still turns solely on enabled, so a +-- recommended row is gated by exactly the same rule as every other row. +-- +-- WHY THIS SEEDS ONE IMAGE AND NOT SEVERAL. The obvious version of this feature +-- — recommend a handful of popular Minecraft images from Docker Hub — ships a +-- trap. Velocity runs modern forwarding, which is a proxy-WIDE setting: with it +-- on, a backend that cannot verify the signed handshake rejects every login the +-- proxy forwards. The operator injects FELIS_FORWARDING_SECRET into every +-- backend pod (operator.buildEnv) but cannot make an image consume it, and an +-- image built from an operator-typed Dockerfile does not. So an arbitrary public +-- image passes admission, builds, schedules, and reports Ready — and then is +-- UNJOINABLE, failing at the last step with nothing in the server's status +-- explaining why. Recommending that is worse than recommending nothing. +-- +-- Grep for FELIS_FORWARDING_SECRET: exactly two images read it in their +-- entrypoints, deploy/limbo and deploy/lobby, and both refuse to start without +-- it. Those two are the entire joinable set. Of them: +-- +-- * limbo is the login gate — a system server pinned to the reserved name +-- "login" plus the setup-owned system-role label, and the only workload that +-- receives FELIS_SERVICE_TOKEN. It has no world and no gameplay; it exists to +-- hold a player at the bind screen. Recommending it as a base for a user's +-- own server would be nonsense. +-- * lobby is Paper plus the felis-paper /menu plugin: a real, joinable, +-- playable server and a sound starting point for a user's own. +-- +-- That leaves exactly one defensible recommendation. Seeding a second entry +-- would mean padding the list with an image that cannot carry a player, so this +-- migration ships the honest set of one. The list grows when Felis ships another +-- forwarding-aware image, not before. +-- +-- REF CAVEAT: felis-lobby:demo is the bootstrap default (FELIS_LOBBY_IMAGE in +-- deploy/bootstrap.sh and deploy/demo-up.sh), built locally and imported into +-- k3s containerd. An install that overrode that variable runs a different ref, +-- and this row will point at an image its cluster does not have. That failure is +-- deliberately the loud kind — the pod ImagePullBackOffs immediately and is +-- visible in server status, rather than starting and silently refusing joins — +-- and an admin clears it with DELETE /images?ref=felis-lobby:demo, which is +-- unvalidated and always works. Re-adding the real ref is NOT symmetric: POST +-- /images runs ValidateImageRef, which requires a host-qualified reference +-- (splitRegistryHost wants a first segment carrying '.' or ':'), so it accepts +-- registry.example:5000/lobby:v2 but REFUSES a bare local containerd tag like +-- my-lobby:v2 — the very shape bootstrap builds. An override that lives only in +-- the node's image store therefore has no API path back in and must be seeded the +-- same way this row was, in SQL. That asymmetry is why this seed is SQL and not a +-- POST. The seed cannot do better on its own: the correct value is operator +-- configuration ([velocity] lobby_image), which is not readable from SQL. +-- +-- added_by records provenance rather than a person: no human admitted this row, +-- the platform did, and the audit trail should say so instead of attributing it +-- to whoever happened to run the migration. +-- +-- Idempotent by ON CONFLICT DO NOTHING: migrations may re-run, and an admin who +-- deliberately disabled or re-pointed this row must not have that decision +-- silently undone on the next apply. +INSERT INTO image_whitelist (image_ref, source, added_by, enabled) +VALUES ('felis-lobby:demo', 'recommended', 'felis-platform', true) +ON CONFLICT (image_ref) DO NOTHING;