diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 367638274..3483b950f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -46,7 +46,9 @@ jobs: - name: 🔐 Validate Flux verification is in effect run: | go test ./scripts/validate-flux-verify - go run ./scripts/validate-flux-verify ksail.prod.yaml + go run ./scripts/validate-flux-verify \ + ksail.prod.yaml \ + k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml # .github/actionlint.yaml silences actionlint's "unexpected key" diagnostic # for concurrency.queue, which also removes its enum check. This restores it. diff --git a/k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml b/k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml index 9828b0a40..4ae9ad303 100644 --- a/k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml +++ b/k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml @@ -147,3 +147,69 @@ spec: options: - name: ndots value: "1" + # Enforce cosign signature verification on the ROOT source — the + # flux-system OCIRepository every controller, tenant binding and policy + # arrives through. Without this the root artifact is pulled from a mutable + # tag with nothing checking who produced it. + # + # PATCH THROUGH THE OPERATOR, not from the cluster config and not from a + # hand-written OCIRepository override. flux-operator generates this + # resource and is its Apply field manager, owning + # url/ref/interval/provider/secretRef. Two consequences, both measured + # live: KSail's spec.workload.flux.verify (ksail.prod.yaml) is applied + # only on the bootstrap path — on a routine deploy KSail writes no spec + # key on this resource at all, so that setting never reaches an existing + # cluster; and a full in-repo override would contend with the operator for + # those five fields on the root source, where losing the reconcile fight + # stops all GitOps delivery. A kustomize patch avoids both: the operator + # itself writes the field, so it lands under the manager that already owns + # the resource — no second writer, no field contention, no SSA pruning + # question. This is flux-operator's documented route for exactly this + # case ("OCI sync with Cosign verification"). + # + # The subjects are pinned to the three workflows that legitimately publish + # this artifact, on the refs they publish from, rather than `@.+` — so a + # signature these same workflows produce on any other ref is not trusted: + # ci.yaml on the merge-queue ref — automatic deploy on merge + # cd.yaml on main — manual workflow_dispatch deploy + # dr-rebuild.yaml on main — disaster recovery, which republishes + # this artifact while rebuilding prod + # from zero; omitting it would leave a + # recovery producing a correctly-signed + # artifact that Flux still refuses. + # They are kept identical to spec.workload.flux.verify in ksail.prod.yaml, + # which covers the bootstrap path; scripts/validate-flux-verify asserts + # both halves on every pull request. + # + # ⚠️ A MISTARGETED KUSTOMIZE PATCH RENDERS NOTHING, EXITS 0 AND WARNS + # NOBODY (measured with kubectl kustomize on this file). A typo in the + # target below yields a security control that is present here, absent from + # the cluster, and green in CI. scripts/validate-flux-verify checks the + # target, and the effect is confirmed by reading the live resource back: + # kubectl get ocirepository/flux-system -n flux-system -o yaml | grep -A5 verify + - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: add + path: /spec/verify + value: + provider: cosign + # 🔴 EXACTLY ONE ENTRY. cosign's keyless verification rejects a + # multi-entry matchOIDCIdentity outright — "unsupported: multiple + # identities are not supported at this time" — and it fails CLOSED + # for the whole set, so a list verifies nothing while looking + # stricter than a single matcher. Flux documents the field as an + # OR'd list and the schema accepts one, so a list is both + # docs-valid and schema-valid and still refuses every artifact. + # Alternation therefore lives INSIDE the subject regex. + # The three trusted signers, in order: ci.yaml (automatic prod + # deploy, signs under the merge-queue ref), cd.yaml (manual + # workflow_dispatch from main), dr-rebuild.yaml (disaster recovery + # from main, which republishes this same artifact). + # scripts/validate-flux-verify fails the build on any list longer + # than one, so this cannot regress silently. + matchOIDCIdentity: + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/(ci\.yaml@refs/heads/gh-readonly-queue/main/.+|(cd|dr-rebuild)\.yaml@refs/heads/main)$' diff --git a/ksail.prod.yaml b/ksail.prod.yaml index e61b88361..88bfa64c1 100644 --- a/ksail.prod.yaml +++ b/ksail.prod.yaml @@ -199,50 +199,59 @@ spec: tag: latest flux: # Cosign keyless signature verification for the flux-system OCIRepository. - # KSail reads this at spec.workload.flux.verify (ksail#4987) and applies it - # to that resource. The manifests artifact is cosign-signed keyless in the - # deploy-prod composite (push → sign → attest), and the subjects below are - # pinned to the production deploy refs rather than `@.+`, so a signature - # these workflows might produce on any other ref is not trusted. + # KSail reads this at spec.workload.flux.verify (ksail#4987). The manifests + # artifact is cosign-signed keyless in the deploy-prod composite + # (push → sign → attest), and the subjects below are pinned to the + # production deploy refs rather than `@.+`, so a signature these workflows + # might produce on any other ref is not trusted. # - # ⚠️ NOT REACHING THE RUNNING CLUSTER — verified 2026-08-04: the live - # ocirepository/flux-system carries no spec.verify. #2922 tracks it. - # The cause is NOT this config: the deploy's `ksail cluster update` step is - # being skipped entirely by the Cilium rollout gate (#2951 — `outcome=skipped - # duration_ms=0` on every prod deploy for days), so the command that would - # apply this has not run. Once #2951 releases that gate this may simply - # start working — flux-operator owns url/ref/interval/provider/secretRef on - # that resource but NOT verify, and Server-Side Apply lets it prune only - # what it owns, so a verify written by KSail should survive. - # Do NOT read it as an active control until you have checked: - # kubectl get ocirepository/flux-system -n flux-system -o yaml | grep -A5 verify + # THIS BLOCK COVERS BOOTSTRAP ONLY — the steady-state control lives in the + # FluxInstance. KSail applies verification from its Flux install path, so + # this is what a cluster gets when it is CREATED. On an existing cluster + # KSail writes no spec key on that resource at all: measured live, + # flux-operator is its Apply field manager (owning + # url/ref/interval/provider/secretRef) while KSail's only field-manager + # entry is the reconcile-trigger annotation. The control that reaches a + # RUNNING cluster is therefore the kustomize patch in + # k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml + # which has the operator itself write spec.verify. Keep the two matcher + # lists identical; scripts/validate-flux-verify asserts both halves on + # every pull request. # # DO NOT "fix" this by hand-writing an in-repo OCIRepository override: a full # override contends with the operator for those five fields on the ROOT # source, where losing the reconcile fight stops all GitOps delivery. # - # Signature enforcement that IS live today: tenant/app OCIRepositories carry - # their own spec.verify, and container images are checked at the Talos - # containerd pull layer plus the Kyverno image-verification policy. The - # infra source is the remaining gap (#1570). + # Signature enforcement elsewhere: tenant/app OCIRepositories carry their + # own spec.verify, and container images are checked at the Talos containerd + # pull layer plus the Kyverno image-verification policy. verify: provider: cosign + # 🔴 EXACTLY ONE ENTRY. cosign's keyless verification rejects a + # multi-entry matchOIDCIdentity outright — "unsupported: multiple + # identities are not supported at this time" — and fails CLOSED for the + # whole set, so a list verifies nothing while looking stricter than a + # single matcher. Flux documents the field as an OR'd list and the schema + # accepts one, so a list is both docs-valid and schema-valid and still + # refuses every artifact. Alternation lives INSIDE the subject regex. + # + # This is the BOOTSTRAP and DISASTER-RECOVERY path, so a broken matcher + # here stays dormant until the moment it is least affordable: a DR + # rebuild would produce a correctly-signed artifact that Flux then + # refuses, with no cluster left to work around it from. + # + # The three trusted signers, alternated in the subject below: + # ci.yaml automatic prod deploy; deploy-prod runs on + # merge_group, signing under the merge-queue ref + # refs/heads/gh-readonly-queue/main/ + # cd.yaml manual prod deploy; workflow_dispatch from main + # dr-rebuild.yaml disaster recovery; workflow_dispatch from main, + # republishing this same artifact + # cd and dr-rebuild are both pinned to refs/heads/main, so a run + # dispatched from any other ref is not trusted. matchOIDCIdentity: - # Automatic prod deploy: ci.yaml deploy-prod runs on merge_group, which - # signs under the merge-queue ref refs/heads/gh-readonly-queue/main/. - issuer: '^https://token\.actions\.githubusercontent\.com$' - subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/ci\.yaml@refs/heads/gh-readonly-queue/main/.+$' - # Manual prod deploy: cd.yaml is workflow_dispatch, run from main. - - issuer: '^https://token\.actions\.githubusercontent\.com$' - subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/cd\.yaml@refs/heads/main$' - # Disaster recovery: dr-rebuild.yaml is workflow_dispatch, run from main, - # and republishes this same artifact while rebuilding prod from zero. - # Without this entry a recovery produces a correctly-signed artifact that - # Flux still refuses — the one situation where a manual workaround is - # least available. Pinned to refs/heads/main, like cd.yaml above, so a DR - # run dispatched from any other ref is not trusted. - - issuer: '^https://token\.actions\.githubusercontent\.com$' - subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/dr-rebuild\.yaml@refs/heads/main$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/(ci\.yaml@refs/heads/gh-readonly-queue/main/.+|(cd|dr-rebuild)\.yaml@refs/heads/main)$' validation: # Parity with ksail.yaml: the prod kustomization pulls the same shared # bases (Coroot + Flagger), so `ksail --config ksail.prod.yaml workload diff --git a/scripts/validate-flux-verify/instance.go b/scripts/validate-flux-verify/instance.go new file mode 100644 index 000000000..d1b520af0 --- /dev/null +++ b/scripts/validate-flux-verify/instance.go @@ -0,0 +1,501 @@ +// This file asserts the half of the contract that reaches the RUNNING cluster. +// +// validate() (main.go) checks ksail.prod.yaml, which is what KSail renders when +// it BOOTSTRAPS the FluxInstance. That path does not run on a routine deploy: +// measured on the live cluster, flux-operator is the Apply manager of +// ocirepository/flux-system and owns url/ref/interval/provider/secretRef, while +// KSail's only field-manager entry on that resource is a reconcile-trigger +// annotation — it writes no spec key there at all. So a correct ksail.prod.yaml +// can sit beside a live root source with spec.verify absent, which is exactly +// the state #2922 recorded. +// +// flux-operator's supported way to add a field to a resource it generates is +// spec.kustomize.patches, and the field then lands under the manager that +// already owns the resource — no second writer, no reconcile fight. That patch +// is therefore the thing that makes verification real, and this is the check +// that it exists and is aimed at the right resource. +// +// 🔴 A MISTARGETED KUSTOMIZE PATCH IS A SILENT NO-OP. +// +// These patches are applied by flux-operator at reconcile time, not by CI, so +// nothing in the pipeline renders them: `kubectl kustomize` copies the patch +// list into the FluxInstance verbatim and reports success whatever the target +// says. Measured separately against a rendered OCIRepository, a target naming +// the wrong kind or the wrong name produces no output, exit 0 and no warning. +// So a typo in the target below yields a security control that is present in +// the file, absent from the cluster, and green in CI — the FluxInstance +// reconciles cleanly and the deploy succeeds either way. +// +// That is why a verify patch pointing anywhere other than the root source is +// reported as a failure here rather than ignored as irrelevant: it is far +// likelier to be the intended control that missed than a deliberate one. +package main + +import ( + "errors" + "fmt" + "reflect" + "sort" + "strings" + + "gopkg.in/yaml.v3" +) + +// rootSourceKind and rootSourceName identify the operator-generated source that +// every controller, tenant binding and policy arrives through. Its name is set +// by flux-operator and is not configurable per-instance. +const ( + rootSourceKind = "OCIRepository" + rootSourceName = "flux-system" + verifyOpPath = "/spec/verify" + + // instanceName/instanceNamespace identify the operator instance this platform + // actually reconciles; confirmed live as fluxinstance/flux in flux-system. + instanceName = "flux" + instanceNamespace = "flux-system" +) + +// writingOps are the JSON-patch operations that PUT a value at a path. `test` +// only asserts and `remove` deletes, so neither delivers the control even +// though both name the same path. +var writingOps = map[string]bool{"add": true, "replace": true} + +// errNoInstance is returned when the manifest carries no FluxInstance, which +// means this check was pointed at the wrong file — a failure of the check's own +// wiring, not of the platform's configuration. +var errNoInstance = errors.New( + "no FluxInstance named flux-system/flux in manifest: this check is pointed at the wrong file, " + + "or the deployed instance was renamed") + +// validateInstance reports why the FluxInstance does not put an effective +// verify block on the live root source, or nil when it does. +func validateInstance(manifest []byte) error { + _, err := instanceVerifyBlock(manifest) + + return err +} + +// instanceVerifyBlock is validateInstance plus the block it validated, so the +// drift check below can compare the two halves without walking the tree a +// second time and inventing a second opinion about where the block lives. +func instanceVerifyBlock(manifest []byte) (map[string]any, error) { + documents, err := decodeAll(manifest) + if err != nil { + return nil, fmt.Errorf("FluxInstance manifest does not parse, so verification cannot be established: %w", err) + } + + instance, err := findInstance(documents) + if err != nil { + return nil, err + } + + patches, _ := lookup(instance, []string{"spec", "kustomize", "patches"}) + + entries, ok := patches.([]any) + if !ok { + entries = nil + } + + // The state is FOLDED over every operation in declared order, not taken + // from the first write. + // + // A patch may `add` /spec/verify and a later operation — in the same body or + // in a later patch on the same target — may `remove` it. Reading the first + // write would report a verified platform whose effective state is no + // verification at all: the same present-but-inert shape this command exists + // to reject, one layer further in. Only the FINAL state is a fact about the + // cluster. + var ( + mistargeted []string + value any + present bool + wrote bool + ) + + for _, entry := range entries { + patch, ok := asMapping(entry) + if !ok { + continue + } + + // Every patch is inspected, so the mistargeted report never depends on + // declaration order. + if !targetsRootSource(patch["target"]) { + if _, _, writes := applyVerifyOps(patch["patch"], nil, false); writes { + mistargeted = append(mistargeted, describeTarget(patch["target"])) + } + + continue + } + + next, nextPresent, writes := applyVerifyOps(patch["patch"], value, present) + value, present = next, nextPresent + wrote = wrote || writes + } + + // Reported before the missing-patch case, because a mistargeted patch is + // the reason the control is usually absent and naming it is the actionable + // half — the same ordering, and the same reasoning, as the stray-block + // check in validate(). + if len(mistargeted) > 0 { + return nil, fmt.Errorf( + "a kustomize patch adds %s but does not target the root source (%s/%s): its target is %s, "+ + "which renders NOTHING and reports no error, so that patch protects nothing: "+ + "point the target at kind %s, name %s, or delete it", + verifyOpPath, rootSourceKind, rootSourceName, strings.Join(mistargeted, ", "), + rootSourceKind, rootSourceName, + ) + } + + if !present { + if wrote { + return nil, fmt.Errorf( + "a kustomize patch writes %s on the %s/%s source and a later operation removes it again, "+ + "so the effective state is NO verification — the block is in the file and absent from "+ + "the cluster: drop the removing operation", + verifyOpPath, rootSourceKind, rootSourceName, + ) + } + + return nil, fmt.Errorf( + "no kustomize patch adds %s to the %s/%s source, so the live root source pulls unverified "+ + "however the cluster config reads (flux-operator owns that resource; a patch is how the "+ + "field reaches it): add a spec.kustomize.patches entry targeting kind %s, name %s", + verifyOpPath, rootSourceKind, rootSourceName, rootSourceKind, rootSourceName, + ) + } + + block, err := checkVerifyValue(value) + if err != nil { + return nil, err + } + + return block, nil +} + +// checkVerifyValue applies the SAME predicates validate() applies to the KSail +// config. Reusing them is the point: two opinions about what an effective +// verify block looks like is how one of them ends up weaker than the other. +func checkVerifyValue(value any) (map[string]any, error) { + block, ok := asMapping(value) + if !ok { + return nil, fmt.Errorf("the %s patch value is not a mapping, so it cannot decode into a verify spec", + verifyOpPath) + } + + provider, isString := block["provider"].(string) + if !enabled(provider) { + reported := any(provider) + if !isString { + reported = block["provider"] + } + + // Flux's own CRD would reject a blank provider outright rather than + // silently disable verification, so this is a stricter superset of what + // the API enforces. It shares validate()'s predicate on purpose: one + // definition of "switched on" across both halves of the contract. + return nil, fmt.Errorf( + "the %s patch sets provider %#v, which is not a usable provider and leaves verification "+ + "DISABLED: set it to cosign", + verifyOpPath, reported, + ) + } + + if !constrainsSigner(block) { + return nil, fmt.Errorf( + "the %s patch is enabled but constrains no signer, so it accepts any keylessly-signed "+ + "artifact from any signer: add a matchOIDCIdentity entry with a non-blank issuer and "+ + "subject, or a secretRef naming a key Secret", + verifyOpPath, + ) + } + + // Asserted here as well as in the cluster-config half, rather than relying on + // checkNoDrift to notice. Drift-checking makes the two halves EQUAL; it does + // not make either CORRECT, so two identically-broken copies pass it. This half + // is also the one that actually reaches a running cluster, so it is the worse + // of the two to leave covered only transitively. + if count := oidcMatcherCount(block["matchOIDCIdentity"]); count > 1 { + return nil, fmt.Errorf( + "the %s patch has %d matchOIDCIdentity entries, and cosign supports exactly ONE: keyless "+ + "verification rejects a multi-entry matcher outright (\"unsupported: multiple identities "+ + "are not supported at this time\") and fails CLOSED for the whole set, so this patch would "+ + "apply a control that verifies NOTHING to the running cluster — collapse the %d subjects "+ + "into one entry using regex alternation inside the subject", + verifyOpPath, count, count, + ) + } + + return block, nil +} + +// checkNoDrift requires the two halves to pin the SAME signers. +// +// The policy is deliberately written twice — once for bootstrap in the cluster +// config, once for the running cluster in the FluxInstance — because the two +// are applied by different components at different times and neither can cover +// the other's window. Two copies of a security policy that nothing compares is +// how one of them quietly stops matching the other: tighten the signer list in +// one place, and a rebuilt cluster keeps trusting whoever the stale copy named, +// with every check still green. Comparing them is what makes the duplication +// safe rather than merely documented. +// +// Only the signer-constraining fields are compared. They are the whole security +// content of the block, and requiring byte-equality of everything would fail on +// differences that change no trust decision. +func checkNoDrift(config, instance map[string]any) error { + for _, field := range []string{"provider", "matchOIDCIdentity", "secretRef"} { + if reflect.DeepEqual(config[field], instance[field]) { + continue + } + + return fmt.Errorf( + "the cluster config and the FluxInstance patch pin different signers, so a bootstrapped "+ + "cluster and a running one would trust different publishers — %q differs:\n%s\n"+ + "make the two verify blocks identical", + field, describeDrift(config[field], instance[field]), + ) + } + + return nil +} + +// describeDrift names the entries that differ rather than dumping both values. +// +// The lists are long, near-identical and full of escaped regex, so printing +// them whole hides the one changed character in a wall of text — and a gate +// whose message cannot be read is a gate people work around instead of fixing. +// Only what is present on one side and missing on the other is shown. +func describeDrift(config, instance any) string { + configItems, configIsList := renderMatchers(config) + instanceItems, instanceIsList := renderMatchers(instance) + + if !configIsList || !instanceIsList { + return fmt.Sprintf(" cluster config: %#v\n FluxInstance patch: %#v", config, instance) + } + + var lines []string + for _, item := range difference(configItems, instanceItems) { + lines = append(lines, " only in the cluster config: "+item) + } + + for _, item := range difference(instanceItems, configItems) { + lines = append(lines, " only in the FluxInstance patch: "+item) + } + + // Same members, different order or count. Order carries no meaning to Flux, + // but the two files still do not match, so say what IS different rather + // than printing an empty list and looking like a false positive. + if len(lines) == 0 { + return fmt.Sprintf(" the same %d entries in a different order or with duplicates", + len(configItems)) + } + + sort.Strings(lines) + + return strings.Join(lines, "\n") +} + +// renderMatchers flattens a matchOIDCIdentity list into comparable strings. +func renderMatchers(value any) ([]string, bool) { + entries, ok := value.([]any) + if !ok { + return nil, false + } + + items := make([]string, 0, len(entries)) + + for _, entry := range entries { + matcher, ok := asMapping(entry) + if !ok { + items = append(items, fmt.Sprintf("%#v", entry)) + + continue + } + + issuer, _ := matcher["issuer"].(string) + subject, _ := matcher["subject"].(string) + items = append(items, fmt.Sprintf("issuer=%s subject=%s", issuer, subject)) + } + + return items, true +} + +// difference returns the members of a that b does not contain. +func difference(a, b []string) []string { + present := make(map[string]bool, len(b)) + for _, item := range b { + present[item] = true + } + + var only []string + + for _, item := range a { + if !present[item] { + only = append(only, item) + } + } + + return only +} + +// findInstance returns the DEPLOYED FluxInstance, selected by identity. +// +// The manifest opens with a comment-only document, which decodes to nil, so +// selecting by content rather than by position is what keeps this pointed at +// the right document. Kind alone is not enough: a second FluxInstance carrying +// a correct verify patch would satisfy every check while the instance that is +// actually deployed carries none — the check would pass on a resource nobody +// runs. Only flux-system/flux is the operator instance this platform reconciles +// (confirmed live), so that is the one identity accepted. +// +// A duplicate is refused rather than resolved. Two documents with this identity +// mean the file no longer says which one wins, and guessing is how a check ends +// up certifying the copy that is not applied. +func findInstance(documents []any) (any, error) { + var found []any + + for _, document := range documents { + mapping, ok := asMapping(document) + if !ok { + continue + } + + kind, _ := mapping["kind"].(string) + if kind != "FluxInstance" { + continue + } + + metadata, _ := asMapping(mapping["metadata"]) + name, _ := metadata["name"].(string) + namespace, _ := metadata["namespace"].(string) + + if name != instanceName || namespace != instanceNamespace { + continue + } + + found = append(found, document) + } + + switch len(found) { + case 0: + return nil, errNoInstance + case 1: + return found[0], nil + default: + return nil, fmt.Errorf( + "%d FluxInstance documents named %s/%s: the file does not say which one is applied, "+ + "so verification cannot be established from it — keep one", + len(found), instanceNamespace, instanceName, + ) + } +} + +// applyVerifyOps folds one patch body's operations over the running +// verifyOpPath state, in declared order, and reports whether it WROTE that +// path. +// +// flux-operator carries a patch body as a STRING holding a JSON6902 op list, so +// it has to be parsed a second time rather than walked as part of the outer +// tree. `add` and `replace` set the value; `remove` clears it; `test` asserts +// and changes nothing. The third return reports whether an add/replace was +// applied — deliberately NOT merely whether the path was mentioned. It is what +// lets the caller tell "configured, then removed" from "never configured": a +// body holding only a `remove` or a `test` never delivered the control, so it +// reads as missing rather than as retracted. +func applyVerifyOps(body, value any, present bool) (any, bool, bool) { + text, ok := body.(string) + if !ok { + return value, present, false + } + + var operations []any + if err := yaml.Unmarshal([]byte(text), &operations); err != nil { + return value, present, false + } + + wrote := false + + for _, item := range operations { + operation, ok := asMapping(item) + if !ok { + continue + } + + path, _ := operation["path"].(string) + if strings.TrimSpace(path) != verifyOpPath { + continue + } + + switch op := strings.TrimSpace(operationName(operation)); { + case writingOps[op]: + value, present, wrote = operation["value"], true, true + case op == "remove": + value, present = nil, false + } + } + + return value, present, wrote +} + +// operationName reads the `op` key, tolerating a non-string so a malformed +// entry is skipped rather than panicking. +func operationName(operation map[string]any) string { + name, _ := operation["op"].(string) + + return name +} + +// targetsRootSource reports whether a patch target names the root source +// specifically. +// +// A target carrying no name matches every OCIRepository the operator generates, +// so the manifest does not state which resource the control lands on. That +// ambiguity is treated as a miss: a control whose subject is inferred rather +// than written is not one this check can certify. +func targetsRootSource(value any) bool { + target, ok := asMapping(value) + if !ok { + return false + } + + kind, _ := target["kind"].(string) + name, _ := target["name"].(string) + + if strings.TrimSpace(kind) != rootSourceKind || strings.TrimSpace(name) != rootSourceName { + return false + } + + // A namespace is optional in a kustomize target — absent means "any", which + // still includes the root source. A namespace that is PRESENT and different + // excludes it, so only that case is a miss. + namespace, isString := target["namespace"].(string) + if isString && strings.TrimSpace(namespace) != "" && strings.TrimSpace(namespace) != rootSourceName { + return false + } + + return true +} + +// describeTarget renders a target for the failure message. The message has to +// name the wrong target concretely, or the reader is told a patch missed +// without being told which one. +func describeTarget(value any) string { + target, ok := asMapping(value) + if !ok { + return "absent" + } + + kind, _ := target["kind"].(string) + name, _ := target["name"].(string) + + if strings.TrimSpace(name) == "" { + name = "" + } + + if strings.TrimSpace(kind) == "" { + kind = "" + } + + return fmt.Sprintf("kind %s, name %s", kind, name) +} diff --git a/scripts/validate-flux-verify/instance_test.go b/scripts/validate-flux-verify/instance_test.go new file mode 100644 index 000000000..de1b4ad9c --- /dev/null +++ b/scripts/validate-flux-verify/instance_test.go @@ -0,0 +1,361 @@ +package main + +import ( + "os" + "strings" + "testing" +) + +// goodInstance is the shape that puts spec.verify on the LIVE root source: a +// flux-operator kustomize patch whose target is the operator-generated +// flux-system OCIRepository. +const goodInstance = ` +apiVersion: fluxcd.controlplane.io/v1 +kind: FluxInstance +metadata: + name: flux + namespace: flux-system +spec: + kustomize: + patches: + - target: + kind: Deployment + name: kustomize-controller + namespace: flux-system + patch: | + - op: replace + path: /spec/replicas + value: 2 + - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: add + path: /spec/verify + value: + provider: cosign + matchOIDCIdentity: + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/.+$' +` + +func TestValidateInstance(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // wantErr is a substring the failure must name, so an arm cannot pass + // on the wrong error. Empty means the manifest must validate. + wantErr string + manifest string + }{ + { + name: "patch on the root source validates", + manifest: goodInstance, + }, + { + // The state this check was written for: every other patch present, + // no verify patch at all, so the live root source pulls unverified + // however correct ksail.prod.yaml is (#2922). + name: "no verify patch is rejected", + manifest: ` +kind: FluxInstance +metadata: + name: flux + namespace: flux-system +spec: + kustomize: + patches: + - target: + kind: Deployment + name: kustomize-controller + patch: | + - op: replace + path: /spec/replicas + value: 2 +`, + wantErr: "no kustomize patch adds /spec/verify", + }, + { + // The silent-no-op shape, measured: a patch whose target matches + // nothing renders NOTHING, exit 0, no warning. The control looks + // configured in the file and does not exist in the cluster. + name: "verify patch aimed at the wrong name is rejected", + manifest: strings.Replace(goodInstance, "name: flux-system\n namespace", "name: NOT-THE-ROOT-SOURCE\n namespace", 1), + wantErr: "does not target the root source", + }, + { + name: "verify patch aimed at the wrong kind is rejected", + manifest: strings.Replace(goodInstance, "kind: OCIRepository", "kind: GitRepository", 1), + wantErr: "does not target the root source", + }, + { + // A target carrying no name matches every OCIRepository in scope, + // so which resource it lands on is not stated by the manifest. + // Ambiguity fails closed here rather than being read as a hit. + name: "verify patch with no target name is rejected", + manifest: strings.Replace(goodInstance, ` + kind: OCIRepository + name: flux-system + namespace: flux-system`, ` + kind: OCIRepository + namespace: flux-system`, 1), + wantErr: "does not target the root source", + }, + { + // The same inert-block class validate() already guards on the KSail + // side: present, enabled, and constraining nobody. + name: "verify patch that constrains no signer is rejected", + manifest: ` +kind: FluxInstance +metadata: + name: flux + namespace: flux-system +spec: + kustomize: + patches: + - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: add + path: /spec/verify + value: + provider: cosign +`, + wantErr: "constrains no signer", + }, + { + // A FluxInstance under another identity is not the one the platform + // reconciles, so a correct patch on it certifies nothing. + name: "a FluxInstance under another identity is not accepted", + manifest: strings.Replace(goodInstance, " name: flux\n", " name: some-other-instance\n", 1), + wantErr: "no FluxInstance", + }, + { + // The decoy case, and the reason kind alone is not enough: the + // instance that IS deployed carries no verify patch while a second + // document does. Selecting by kind would pass this. + name: "a decoy instance cannot satisfy the check for the real one", + manifest: strings.Replace(goodInstance, " name: flux\n", " name: decoy\n", 1) + ` +--- +kind: FluxInstance +metadata: + name: flux + namespace: flux-system +spec: + kustomize: + patches: [] +`, + wantErr: "no kustomize patch adds /spec/verify", + }, + { + name: "duplicate FluxInstance documents are rejected", + manifest: goodInstance + "\n---" + goodInstance, + wantErr: "does not say which one is applied", + }, + { + // add-then-remove WITHIN one patch body. + name: "add then remove in one patch body is rejected", + manifest: ` +kind: FluxInstance +metadata: + name: flux + namespace: flux-system +spec: + kustomize: + patches: + - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: add + path: /spec/verify + value: + provider: cosign + matchOIDCIdentity: + - issuer: 'https://example.invalid' + subject: 'https://example.invalid/workflow' + - op: remove + path: /spec/verify +`, + wantErr: "removes it again", + }, + { + // 🔴 THE SHAPE THAT HALTED PROD, pinned on the half that actually + // reaches a running cluster. Three individually well-formed entries: + // the patch targets correctly, the provider is cosign, and a signer is + // constrained — so every other assertion in this file passes. cosign + // still refuses every artifact, because a multi-entry matcher fails + // CLOSED for the whole set. + // + // Asserted here as well as in the cluster-config half deliberately. + // checkNoDrift only makes the two halves EQUAL, so two identically + // broken copies satisfy it; equality is not correctness. + name: "a multi-entry matchOIDCIdentity is rejected", + manifest: ` +kind: FluxInstance +metadata: + name: flux + namespace: flux-system +spec: + kustomize: + patches: + - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: add + path: /spec/verify + value: + provider: cosign + matchOIDCIdentity: + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/ci\.yaml@refs/heads/gh-readonly-queue/main/.+$' + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/cd\.yaml@refs/heads/main$' + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/dr-rebuild\.yaml@refs/heads/main$' +`, + wantErr: "supports exactly ONE", + }, + { + // add-then-remove ACROSS two patches on the same target: the first + // write says verified, the effective state is not. + name: "a later patch removing the field is rejected", + manifest: goodInstance + ` - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: remove + path: /spec/verify +`, + wantErr: "removes it again", + }, + { + // The LAST write wins, so a later replace that constrains nobody is + // what gets judged — not the well-formed earlier one. + name: "a later replace that constrains no signer is rejected", + manifest: goodInstance + ` - target: + kind: OCIRepository + name: flux-system + namespace: flux-system + patch: | + - op: replace + path: /spec/verify + value: + provider: cosign +`, + wantErr: "constrains no signer", + }, + { + // remove-then-add leaves the field PRESENT, so it must pass: + // rejecting it would fail a legitimate reset-and-set. + name: "remove then add is accepted", + manifest: strings.Replace(goodInstance, + " - op: add\n path: /spec/verify", + " - op: remove\n path: /spec/verify\n - op: add\n path: /spec/verify", + 1), + }, + { + // Mirrors KSail's own Enabled() predicate: a blank provider renders + // no verification at all. + name: "verify patch with a blank provider is rejected", + manifest: strings.Replace(goodInstance, "provider: cosign", `provider: " "`, 1), + wantErr: "DISABLED", + }, + { + // `remove` would delete the field this check exists to require, and + // `test` asserts rather than writes. Only a write counts. + name: "a non-writing op on /spec/verify is rejected", + manifest: strings.Replace(goodInstance, "op: add", "op: remove", 1), + wantErr: "no kustomize patch adds /spec/verify", + }, + { + name: "a manifest with no FluxInstance is rejected", + manifest: ` +kind: ConfigMap +metadata: + name: not-a-flux-instance +`, + wantErr: "no FluxInstance", + }, + { + name: "unparseable manifest is rejected", + manifest: "\tthis: [is not\n", + wantErr: "does not parse", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + err := validateInstance([]byte(test.manifest)) + + switch { + case test.wantErr == "" && err != nil: + t.Fatalf("manifest should validate, got: %v", err) + case test.wantErr != "" && err == nil: + t.Fatalf("manifest should be rejected naming %q, got nil", test.wantErr) + case test.wantErr != "" && !strings.Contains(err.Error(), test.wantErr): + t.Fatalf("error should name %q, got: %v", test.wantErr, err) + } + }) + } +} + +// TestRealFluxInstanceValidates pins the checked-in manifest itself, so the +// contract is asserted against what actually ships rather than only against +// fixtures. The path is relative to this package's directory. +func TestRealFluxInstanceValidates(t *testing.T) { + t.Parallel() + + const path = "../../k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml" + + manifest, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + + if err := validateInstance(manifest); err != nil { + t.Fatalf("the shipped FluxInstance should carry an effective verify patch: %v", err) + } +} + +// TestRealHalvesAgree pins the SHIPPED pair against each other. The two files +// are the ones CI passes, so this asserts the live duplication is consistent +// rather than only that each file is individually well-formed. +func TestRealHalvesAgree(t *testing.T) { + t.Parallel() + + config, err := os.ReadFile("../../ksail.prod.yaml") + if err != nil { + t.Fatalf("read cluster config: %v", err) + } + + manifest, err := os.ReadFile( + "../../k8s/providers/hetzner/infrastructure/controllers/flux-instance/flux-instance.yaml") + if err != nil { + t.Fatalf("read FluxInstance: %v", err) + } + + configBlock, err := configVerifyBlock(config) + if err != nil { + t.Fatalf("cluster config half: %v", err) + } + + instanceBlock, err := instanceVerifyBlock(manifest) + if err != nil { + t.Fatalf("FluxInstance half: %v", err) + } + + if err := checkNoDrift(configBlock, instanceBlock); err != nil { + t.Fatalf("the two halves must pin the same signers: %v", err) + } +} diff --git a/scripts/validate-flux-verify/main.go b/scripts/validate-flux-verify/main.go index 9f6a08396..993a523f9 100644 --- a/scripts/validate-flux-verify/main.go +++ b/scripts/validate-flux-verify/main.go @@ -109,6 +109,32 @@ func namesSecret(value any) bool { return strings.TrimSpace(name) != "" } +// oidcMatcherCount reports how many entries the matchOIDCIdentity list carries, +// and 0 for anything that is not a list. +// +// 🔴 MORE THAN ONE ENTRY VERIFIES NOTHING. cosign's keyless path rejects a +// multi-entry matcher outright — "unsupported: multiple identities are not +// supported at this time" — and it fails CLOSED for the entire set rather than +// falling back to the first entry. Flux documents the field as an OR'd list and +// the CRD schema accepts one, so such a block is docs-valid, schema-valid, +// present at the right path, enabled, and constrains a signer — it passes every +// other check in this file while refusing every artifact. +// +// That is not hypothetical: a three-entry matcher on the root OCIRepository +// halted all GitOps delivery on prod for hours, and the config was green in CI +// throughout. This count is the check that would have caught it, which is why +// it is asserted separately from hasUsableOIDCMatcher below — that one asks +// whether ANY entry is usable, and is deliberately tolerant of extra entries. +// Tolerance is right for judging usability and wrong for judging arity. +func oidcMatcherCount(value any) int { + entries, ok := value.([]any) + if !ok { + return 0 + } + + return len(entries) +} + // hasUsableOIDCMatcher reports whether at least ONE matcher entry pins both an // issuer and a subject. // @@ -270,9 +296,17 @@ func decodeAll(config []byte) ([]any, error) { // validate reports why the config's signature verification is not in effect, or // nil when it is. func validate(config []byte) error { + _, err := configVerifyBlock(config) + + return err +} + +// configVerifyBlock is validate() plus the block it validated, so the drift +// check can compare the two halves without re-walking the tree. +func configVerifyBlock(config []byte) (map[string]any, error) { documents, err := decodeAll(config) if err != nil { - return fmt.Errorf("cluster config does not parse, so verification cannot be established: %w", err) + return nil, fmt.Errorf("cluster config does not parse, so verification cannot be established: %w", err) } // A cluster config is ONE `kind: Cluster` resource, so everything after the @@ -282,7 +316,7 @@ func validate(config []byte) error { // document one plus a stray block in document two validated clean. Refusing // the shape is simpler than guessing which document was meant to win. if len(documents) > 1 { - return fmt.Errorf( + return nil, fmt.Errorf( "cluster config has %d YAML documents; only the first is read as the cluster spec, "+ "so anything configured in the rest verifies nothing: keep it to one document", len(documents), @@ -297,7 +331,7 @@ func validate(config []byte) error { // Reported before the read-path check, because a stray block is the reason // the read path is usually empty and naming it is the actionable half. if stray := strayVerifyPaths(tree, nil); len(stray) > 0 { - return fmt.Errorf( + return nil, fmt.Errorf( "verify block at %s, which KSail does not read: move it to %s or delete it "+ "(a block at any other path is discarded in silence and verifies nothing)", strings.Join(stray, ", "), strings.Join(readPath, "."), @@ -306,7 +340,7 @@ func validate(config []byte) error { value, ok := lookup(tree, readPath) if !ok { - return fmt.Errorf( + return nil, fmt.Errorf( "no verify block at %s, so the generated flux-system OCIRepository pulls unverified: "+ "add a cosign block there", strings.Join(readPath, "."), @@ -315,7 +349,7 @@ func validate(config []byte) error { block, ok := asMapping(value) if !ok { - return fmt.Errorf("%s is not a mapping, so KSail cannot decode it into a verify spec", + return nil, fmt.Errorf("%s is not a mapping, so KSail cannot decode it into a verify spec", strings.Join(readPath, ".")) } @@ -331,7 +365,7 @@ func validate(config []byte) error { reported = block["provider"] } - return fmt.Errorf( + return nil, fmt.Errorf( "%s.provider is %#v, which KSail treats as verification DISABLED "+ "(it renders spec.verify only when the provider is a non-blank string): set it to cosign", strings.Join(readPath, "."), reported, @@ -341,7 +375,7 @@ func validate(config []byte) error { // Reported last, because it is the only check here that assumes the block is // otherwise well-formed and switched on. if !constrainsSigner(block) { - return fmt.Errorf( + return nil, fmt.Errorf( "%s is enabled but constrains no signer, so it accepts any keylessly-signed artifact "+ "from any signer (Flux reads a missing secretRef as keyless): add a matchOIDCIdentity "+ "entry with a non-blank issuer and subject, or a secretRef naming a key Secret "+ @@ -350,12 +384,30 @@ func validate(config []byte) error { ) } - return nil + if count := oidcMatcherCount(block["matchOIDCIdentity"]); count > 1 { + return nil, fmt.Errorf( + "%s.matchOIDCIdentity has %d entries, and cosign supports exactly ONE: keyless "+ + "verification rejects a multi-entry matcher outright (\"unsupported: multiple "+ + "identities are not supported at this time\") and fails CLOSED for the whole set, "+ + "so the block verifies NOTHING while reading as stricter than a single matcher — "+ + "collapse the %d subjects into one entry using regex alternation inside the subject, "+ + "e.g. '^https://github\\.com/org/repo/\\.github/workflows/(a|b)\\.yaml@refs/heads/main$'", + strings.Join(readPath, "."), count, count, + ) + } + + return block, nil } +// run checks BOTH halves of the contract, and requires both paths rather than +// making the second optional. The two configure different lifecycle stages — +// the cluster config covers bootstrap, the FluxInstance patch covers the +// running cluster — and either alone leaves a real window unverified. An +// optional second argument would let a caller silently drop the half that #2922 +// was actually about, which is the failure this command exists to make loud. func run(args []string, stderr io.Writer) int { - if len(args) != 1 { - _, _ = fmt.Fprintln(stderr, "usage: validate-flux-verify ") + if len(args) != 2 { + _, _ = fmt.Fprintln(stderr, "usage: validate-flux-verify ") return 1 } @@ -367,7 +419,31 @@ func run(args []string, stderr io.Writer) int { return 1 } - if err := validate(config); err != nil { + configBlock, err := configVerifyBlock(config) + if err != nil { + _, _ = fmt.Fprintf(stderr, "flux verify contract: %v\n", err) + + return 1 + } + + manifest, err := os.ReadFile(args[1]) //nolint:gosec // Explicit path from the caller. + if err != nil { + _, _ = fmt.Fprintf(stderr, "flux verify contract: read FluxInstance manifest: %v\n", err) + + return 1 + } + + instanceBlock, err := instanceVerifyBlock(manifest) + if err != nil { + _, _ = fmt.Fprintf(stderr, "flux verify contract: %v\n", err) + + return 1 + } + + // Last, because it is the only check that assumes BOTH halves are already + // well-formed: comparing a block that failed its own checks would report + // drift where the real fault is the block itself. + if err := checkNoDrift(configBlock, instanceBlock); err != nil { _, _ = fmt.Fprintf(stderr, "flux verify contract: %v\n", err) return 1 diff --git a/scripts/validate-flux-verify/main_test.go b/scripts/validate-flux-verify/main_test.go index a2feafa79..575e65d34 100644 --- a/scripts/validate-flux-verify/main_test.go +++ b/scripts/validate-flux-verify/main_test.go @@ -301,10 +301,21 @@ spec: wantErr: "constrains no signer", }, { - // One usable entry is enough. A commented-out or half-written sibling - // must not veto a block that does constrain a signer, or the gate - // starts failing correct configs. - name: "one complete matcher among incomplete ones validates", + // 🔴 THIS ARM WAS INVERTED, and the reason matters more than the arm. + // + // It previously expected this config to VALIDATE, reasoning that one + // usable entry is enough and a half-written sibling must not veto a + // block that does constrain a signer. That reasoning describes Flux's + // documented OR semantics — and cosign does not implement them. Keyless + // verification rejects any multi-entry matcher outright and fails CLOSED + // for the whole set, so the "usable" entry below never gets a chance to + // match. The old expectation therefore blessed a block that verifies + // NOTHING. + // + // Not a hypothetical correction: a three-entry matcher on the root + // OCIRepository halted all GitOps delivery on prod for hours while every + // check, including this validator, stayed green. + name: "a second matcher entry is rejected even when one entry is complete", config: ` spec: workload: @@ -316,6 +327,44 @@ spec: subject: '' - issuer: '^https://token\.actions\.githubusercontent\.com$' subject: '^https://github\.com/devantler-tech/platform/.+$' +`, + wantErr: "supports exactly ONE", + }, + { + // The exact shape that took prod down, pinned so it cannot return: three + // well-formed entries, each individually correct, collectively inert. + // Every other check in this file passes on it. + name: "the three-entry prod matcher is rejected", + config: ` +spec: + workload: + flux: + verify: + provider: cosign + matchOIDCIdentity: + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/ci\.yaml@refs/heads/gh-readonly-queue/main/.+$' + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/cd\.yaml@refs/heads/main$' + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/dr-rebuild\.yaml@refs/heads/main$' +`, + wantErr: "has 3 entries", + }, + { + // The non-vacuity control for the two arms above: the SAME three signers, + // alternated inside one subject, must still validate. Without this a + // guard that rejected every matcher would look equally green. + name: "the three signers alternated in one subject validate", + config: ` +spec: + workload: + flux: + verify: + provider: cosign + matchOIDCIdentity: + - issuer: '^https://token\.actions\.githubusercontent\.com$' + subject: '^https://github\.com/devantler-tech/platform/\.github/workflows/(ci\.yaml@refs/heads/gh-readonly-queue/main/.+|(cd|dr-rebuild)\.yaml@refs/heads/main)$' `, }, { @@ -455,16 +504,42 @@ func TestRunExitCodes(t *testing.T) { t.Fatalf("write fixture: %v", err) } + instance := filepath.Join(dir, "instance.yaml") + if err := os.WriteFile(instance, []byte(goodInstance), 0o600); err != nil { + t.Fatalf("write fixture: %v", err) + } + + drifted := filepath.Join(dir, "drifted.yaml") + if err := os.WriteFile(drifted, []byte(strings.Replace( + goodInstance, `platform/.+$`, `platform/some-other-workflow\.yaml@refs/heads/main$`, 1, + )), 0o600); err != nil { + t.Fatalf("write fixture: %v", err) + } + + unpatched := filepath.Join(dir, "unpatched.yaml") + if err := os.WriteFile(unpatched, []byte("kind: FluxInstance\nspec:\n kustomize:\n patches: []\n"), 0o600); err != nil { + t.Fatalf("write fixture: %v", err) + } + tests := []struct { name string args []string want int }{ - {name: "valid config exits 0", args: []string{good}, want: 0}, - {name: "misplaced block exits 1", args: []string{bad}, want: 1}, - {name: "missing file exits 1", args: []string{filepath.Join(dir, "absent.yaml")}, want: 1}, + {name: "valid config and instance exits 0", args: []string{good, instance}, want: 0}, + // Both halves are individually valid and they trust DIFFERENT + // publishers, so a rebuilt cluster and a running one would disagree + // about who may sign. Neither half's own checks can see this. + {name: "drifted signer lists exit 1", args: []string{good, drifted}, want: 1}, + {name: "misplaced block exits 1", args: []string{bad, instance}, want: 1}, + // The half #2922 is about: a correct cluster config beside a + // FluxInstance that never puts verify on the live root source. + {name: "unpatched instance exits 1", args: []string{good, unpatched}, want: 1}, + {name: "missing config file exits 1", args: []string{filepath.Join(dir, "absent.yaml"), instance}, want: 1}, + {name: "missing instance file exits 1", args: []string{good, filepath.Join(dir, "absent.yaml")}, want: 1}, {name: "no arguments exits 1", args: nil, want: 1}, - {name: "two arguments exits 1", args: []string{good, good}, want: 1}, + {name: "one argument exits 1", args: []string{good}, want: 1}, + {name: "three arguments exits 1", args: []string{good, instance, good}, want: 1}, } for _, test := range tests {