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{}