From 1b51165e881d822f67f93c1ede4d80a1b0c4768e Mon Sep 17 00:00:00 2001 From: Tuomas Katila Date: Mon, 31 Aug 2026 11:00:14 +0300 Subject: [PATCH] fwupdate: make content image verification generic ContentImageVerifier no longer speaks in API types nor knows anything about firmware. Its single method is VerifyImage(ctx, ImageVerifyRequest), where the request carries the image reference, pull secret, TLS setting and the expected files as plain fields, so any caller can use the verifier without owning a GPUFirmwareUpdate. An empty Files list means "reachability only" and resolves the manifest with remote.Head instead of pulling every layer and streaming the export. Signed-off-by: Tuomas Katila --- internal/controller/contentimage_verifier.go | 104 +++++++++++++----- .../controller/contentimage_verifier_test.go | 8 ++ .../gpufirmwareupdate_controller.go | 14 ++- .../gpufirmwareupdate_controller_test.go | 32 +++--- 4 files changed, 116 insertions(+), 42 deletions(-) diff --git a/internal/controller/contentimage_verifier.go b/internal/controller/contentimage_verifier.go index 62970a4..a1bd4e6 100644 --- a/internal/controller/contentimage_verifier.go +++ b/internal/controller/contentimage_verifier.go @@ -25,6 +25,7 @@ import ( "fmt" "io" "net/http" + "path" "strings" "github.com/google/go-containerregistry/pkg/authn" @@ -34,14 +35,44 @@ import ( core "k8s.io/api/core/v1" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" - - v1alpha "github.com/intel/gpu-base-operator/api/v1alpha1" ) -// ContentImageVerifier checks whether the firmware content image is reachable and, -// when checksums are declared, verifies each file's SHA256 against the image contents. +// ContentImageVerifier checks whether an image is reachable and, when checksums are declared, +// verifies each file's SHA256 against the image contents. type ContentImageVerifier interface { - Verify(ctx context.Context, spec *v1alpha.GPUFirmwareUpdateSpec) error + VerifyImage(ctx context.Context, req ImageVerifyRequest) error +} + +// ImageVerifyRequest describes one image to check. Callers assemble it from wherever they keep +// their image reference and pull settings, so a caller holding several images in unrelated fields +// can ask for a different depth of check for each. +type ImageVerifyRequest struct { + // Image is the reference to check. + Image string + + // PullSecret names a dockerconfigjson Secret in the operator namespace, or "" to use the + // ambient keychain. + PullSecret string + + // InsecureSkipTLSVerify disables registry certificate validation. + InsecureSkipTLSVerify bool + + // Files, when non-empty must exist in the image, and are SHA256-verified wherever a checksum is declared. + // If no files are declared, only the image reference is checked for reachability and authentication. + Files []ImageFile +} + +// ImageFile is a file the verifier expects to find in the image. It is deliberately not an API +// type: the verifier only ever needs a name and an optional checksum, so callers convert from +// whatever their CRD happens to call those fields. +type ImageFile struct { + // Name is the file's path in the image's merged filesystem, e.g. "/fwupdate/gfx.bin". + // A leading "/" or "./" is optional. + Name string + + // Checksum is the expected SHA256 as "sha256:<64 hex characters>". Empty means the file's + // existence is checked but its contents are not. + Checksum string } // DefaultContentImageVerifier implements ContentImageVerifier using the OCI registry API. @@ -57,15 +88,16 @@ func newContentImageVerifier(k8sReader client.Reader, namespace string) ContentI } } -// Verify checks image reachability. All files are checked for existence. If a file declares a Checksum, the -// checksum is verified against the /fwupdate/ content. -func (v *DefaultContentImageVerifier) Verify(ctx context.Context, spec *v1alpha.GPUFirmwareUpdateSpec) error { - ref, err := name.ParseReference(spec.Content.ContainerImage) +// VerifyImage checks one image against the registry. With no Files it confirms only that the +// reference parses, authenticates and resolves. With Files it additionally streams the merged +// filesystem and checks each named file exists, verifying SHA256 where declared. +func (v *DefaultContentImageVerifier) VerifyImage(ctx context.Context, req ImageVerifyRequest) error { + ref, err := name.ParseReference(req.Image) if err != nil { - return fmt.Errorf("invalid content image reference %q: %w", spec.Content.ContainerImage, err) + return fmt.Errorf("invalid content image reference %q: %w", req.Image, err) } - keychain, err := v.buildKeychain(ctx, spec.ImagePullSecret) + keychain, err := v.buildKeychain(ctx, req.PullSecret) if err != nil { return fmt.Errorf("failed to build registry auth: %w", err) } @@ -75,7 +107,7 @@ func (v *DefaultContentImageVerifier) Verify(ctx context.Context, spec *v1alpha. remote.WithContext(ctx), } - if spec.InsecureSkipTLSVerify { + if req.InsecureSkipTLSVerify { defaultTransport, ok := http.DefaultTransport.(*http.Transport) if !ok { return fmt.Errorf("unexpected default transport type: %T", http.DefaultTransport) @@ -94,10 +126,21 @@ func (v *DefaultContentImageVerifier) Verify(ctx context.Context, spec *v1alpha. remoteOpts = append(remoteOpts, remote.WithTransport(insecureTransport)) } + // Reachability only: resolve the manifest and stop. Deliberately not remote.Image followed by + // an export — that would download every layer to answer a question the descriptor already + // answers, and the caller asking for this depth has no file it wants to look at. + if len(req.Files) == 0 { + if _, err := remote.Head(ref, remoteOpts...); err != nil { + return fmt.Errorf("failed to resolve content image %q: %w", req.Image, err) + } + + return nil + } + // Full checksum verification: pull and stream the merged filesystem. img, err := remote.Image(ref, remoteOpts...) if err != nil { - return fmt.Errorf("failed to pull content image %q: %w", spec.Content.ContainerImage, err) + return fmt.Errorf("failed to pull content image %q: %w", req.Image, err) } pr, pw := io.Pipe() @@ -112,7 +155,7 @@ func (v *DefaultContentImageVerifier) Verify(ctx context.Context, spec *v1alpha. } }() - verifyErr := verifyChecksumsFromExport(pr, spec.Content.Files) + verifyErr := verifyChecksumsFromExport(pr, req.Files) // Closing the read end unblocks the export goroutine if it is still running. if closeErr := pr.Close(); closeErr != nil && verifyErr == nil { @@ -123,15 +166,21 @@ func (v *DefaultContentImageVerifier) Verify(ctx context.Context, spec *v1alpha. } // verifyChecksumsFromExport reads a merged-filesystem tar (as produced by crane.Export) -// and checks that each firmware file is present. If a firmware file declares a Checksum, -// it verifies the SHA256 of that file. +// and checks that each file is present. If a file declares a Checksum, it verifies the +// SHA256 of that file. // It returns a distinct error for a missing file versus a checksum mismatch. -func verifyChecksumsFromExport(tarStream io.Reader, files []v1alpha.GPUFirmwareFile) error { +func verifyChecksumsFromExport(tarStream io.Reader, files []ImageFile) error { want := make(map[string]string, len(files)) for _, f := range files { + n := normalizeImagePath(f.Name) + + if _, exists := want[n]; exists { + return fmt.Errorf("duplicate file %q in verification request", f.Name) + } + // Empty Checksum is treated as "not available" - want[f.FileName] = f.Checksum + want[n] = f.Checksum } found := map[string]bool{} @@ -148,13 +197,7 @@ func verifyChecksumsFromExport(tarStream io.Reader, files []v1alpha.GPUFirmwareF return fmt.Errorf("error reading image filesystem: %w", err) } - // crane.Export paths look like "fwupdate/file.bin" or "./fwupdate/file.bin". - base := strings.TrimPrefix(hdr.Name, "./") - if !strings.HasPrefix(base, "fwupdate/") { - continue - } - - base = strings.TrimPrefix(base, "fwupdate/") + base := normalizeImagePath(hdr.Name) expected, ok := want[base] if !ok { @@ -163,7 +206,7 @@ func verifyChecksumsFromExport(tarStream io.Reader, files []v1alpha.GPUFirmwareF if expected != "" { if hdr.Typeflag != tar.TypeReg { - return fmt.Errorf("firmware file %q found in image but is not a regular file", base) + return fmt.Errorf("file %q found in image but is not a regular file", base) } h := sha256.New() @@ -191,13 +234,20 @@ func verifyChecksumsFromExport(tarStream io.Reader, files []v1alpha.GPUFirmwareF for filename := range want { if !found[filename] { - return fmt.Errorf("firmware file %q not found under /fwupdate/ in the content image", filename) + return fmt.Errorf("file %q not found in the content image", filename) } } return nil } +// normalizeImagePath makes tar entry names and requested file names comparable. crane.Export +// writes paths as "fwupdate/file.bin" or "./fwupdate/file.bin", while a caller naming a file in +// the image is likely to write "/fwupdate/file.bin". +func normalizeImagePath(p string) string { + return strings.TrimPrefix(path.Clean("/"+p), "/") +} + // dockerConfigJSON mirrors the .dockerconfigjson secret format. type dockerConfigJSON struct { Auths map[string]authn.AuthConfig `json:"auths"` diff --git a/internal/controller/contentimage_verifier_test.go b/internal/controller/contentimage_verifier_test.go index a4bc226..e3c5445 100644 --- a/internal/controller/contentimage_verifier_test.go +++ b/internal/controller/contentimage_verifier_test.go @@ -81,4 +81,12 @@ var _ = Describe("ContentImageVerifier", func() { Expect(kc).To(BeNil()) }) }) + + Context("normalize image path", func() { + It("should normalize image path", func() { + Expect(normalizeImagePath("/fwupdate/file.bin")).To(Equal("fwupdate/file.bin")) + Expect(normalizeImagePath("./fwupdate/file.bin")).To(Equal("fwupdate/file.bin")) + Expect(normalizeImagePath("fwupdate/file.bin")).To(Equal("fwupdate/file.bin")) + }) + }) }) diff --git a/internal/controller/gpufirmwareupdate_controller.go b/internal/controller/gpufirmwareupdate_controller.go index a03655a..bdcced0 100644 --- a/internal/controller/gpufirmwareupdate_controller.go +++ b/internal/controller/gpufirmwareupdate_controller.go @@ -310,8 +310,20 @@ func (r *GPUFirmwareUpdateReconciler) selectNodesToUpdate(ctx context.Context, f return selected } +// verifyContentImage checks that the firmware content image is reachable and that every declared +// firmware file exists in it, verifying the SHA256 of the files that declare a checksum. func (r *GPUFirmwareUpdateReconciler) verifyContentImage(ctx context.Context, fu *intelcomv1alpha1.GPUFirmwareUpdate) error { - return r.imgVerify.Verify(ctx, &fu.Spec) + files := make([]ImageFile, 0, len(fu.Spec.Content.Files)) + for _, fw := range fu.Spec.Content.Files { + files = append(files, ImageFile{Name: fmt.Sprintf("/fwupdate/%s", fw.FileName), Checksum: fw.Checksum}) + } + + return r.imgVerify.VerifyImage(ctx, ImageVerifyRequest{ + Image: fu.Spec.Content.ContainerImage, + PullSecret: fu.Spec.ImagePullSecret, + InsecureSkipTLSVerify: fu.Spec.InsecureSkipTLSVerify, + Files: files, + }) } func (r *GPUFirmwareUpdateReconciler) beginUpdate(ctx context.Context, fu *intelcomv1alpha1.GPUFirmwareUpdate) (ctrl.Result, error) { diff --git a/internal/controller/gpufirmwareupdate_controller_test.go b/internal/controller/gpufirmwareupdate_controller_test.go index bbb0a72..eb182ac 100644 --- a/internal/controller/gpufirmwareupdate_controller_test.go +++ b/internal/controller/gpufirmwareupdate_controller_test.go @@ -269,12 +269,16 @@ func (mlr *MockLogRetriever) RetrieveLogsForPodContainer(ctx context.Context, po }, nil } -// fakeContentImageVerifier is a test double for ContentImageVerifier. +// fakeContentImageVerifier is a test double for ContentImageVerifier. requests records the +// VerifyImage calls so a test can assert on the depth of check that was asked for. type fakeContentImageVerifier struct { - err error + err error + requests []ImageVerifyRequest } -func (f *fakeContentImageVerifier) Verify(_ context.Context, _ *intelcomv1alpha1.GPUFirmwareUpdateSpec) error { +func (f *fakeContentImageVerifier) VerifyImage(_ context.Context, req ImageVerifyRequest) error { + f.requests = append(f.requests, req) + return f.err } @@ -1048,8 +1052,8 @@ var _ = Describe("verifyChecksumsFromExport", func() { It("passes when all declared checksums match", func() { content := []byte("firmware binary data") - files := []intelcomv1alpha1.GPUFirmwareFile{ - {Type: "GFX", FileName: "gfx.bin", Checksum: sha256Of(content)}, + files := []ImageFile{ + {Name: "/fwupdate/gfx.bin", Checksum: sha256Of(content)}, } err := verifyChecksumsFromExport(makeTar(map[string][]byte{"gfx.bin": content}), files) @@ -1058,9 +1062,9 @@ var _ = Describe("verifyChecksumsFromExport", func() { It("passes when some files have no checksum (those are skipped)", func() { content := []byte("firmware binary data") - files := []intelcomv1alpha1.GPUFirmwareFile{ - {Type: "GFX", FileName: "gfx.bin", Checksum: sha256Of(content)}, - {Type: "GFX_DATA", FileName: "gfxdata.bin"}, // no checksum — skipped + files := []ImageFile{ + {Name: "/fwupdate/gfx.bin", Checksum: sha256Of(content)}, + {Name: "/fwupdate/gfxdata.bin"}, // no checksum — skipped } tarFiles := map[string][]byte{ "gfx.bin": content, @@ -1072,8 +1076,8 @@ var _ = Describe("verifyChecksumsFromExport", func() { }) It("returns a mismatch error when checksum does not match", func() { - files := []intelcomv1alpha1.GPUFirmwareFile{ - {Type: "GFX", FileName: "gfx.bin", Checksum: sha256Of([]byte("correct data"))}, + files := []ImageFile{ + {Name: "/fwupdate/gfx.bin", Checksum: sha256Of([]byte("correct data"))}, } err := verifyChecksumsFromExport(makeTar(map[string][]byte{"gfx.bin": []byte("wrong data")}), files) @@ -1083,8 +1087,8 @@ var _ = Describe("verifyChecksumsFromExport", func() { }) It("returns a distinct error when a declared file is absent from the image", func() { - files := []intelcomv1alpha1.GPUFirmwareFile{ - {Type: "GFX", FileName: "missing.bin", Checksum: sha256Of([]byte("data"))}, + files := []ImageFile{ + {Name: "/fwupdate/missing.bin", Checksum: sha256Of([]byte("data"))}, } err := verifyChecksumsFromExport(makeTar(map[string][]byte{"other.bin": []byte("data")}), files) @@ -1095,8 +1099,8 @@ var _ = Describe("verifyChecksumsFromExport", func() { It("ignores files outside the fwupdate/ directory", func() { content := []byte("firmware") - files := []intelcomv1alpha1.GPUFirmwareFile{ - {Type: "GFX", FileName: "gfx.bin", Checksum: sha256Of(content)}, + files := []ImageFile{ + {Name: "/fwupdate/gfx.bin", Checksum: sha256Of(content)}, } // Tar contains the file at the correct path AND a decoy at the wrong path. buf := &bytes.Buffer{}