In-memory OCI artifact pull

CRI-O now pulls OCI artifacts directly in-memory after the discussions in containers/image#2306 and kubernetes/website#45121. CRI-O also enforces the config media type `application/vnd.cncf.seccomp-profile.config.v1+json` for seccomp profiles. Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert · Feb 26, 2024 · 3a026aa · 3a026aa
1 parent 6ff7c53
commit 3a026aa
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 109 deletions.
diff --git a/internal/config/ociartifact/ociartifact.go b/internal/config/ociartifact/ociartifact.go
@@ -3,18 +3,20 @@ package ociartifact
 import (
 	"context"
 	"fmt"
+	"io"
 
 	"github.com/containers/common/libimage"
-	"github.com/containers/common/pkg/config"
+	"github.com/containers/image/v5/docker"
+	"github.com/containers/image/v5/manifest"
+	"github.com/containers/image/v5/pkg/blobinfocache"
 	"github.com/containers/image/v5/types"
-	"github.com/containers/storage"
 
 	"github.com/cri-o/cri-o/internal/log"
 )
 
 // Impl is the main implementation interface of this package.
 type Impl interface {
-	Pull(context.Context, *types.SystemContext, string) (*Artifact, error)
+	Pull(context.Context, *types.SystemContext, string, string) (*Artifact, error)
 }
 
 // New returns a new OCI artifact implementation.
@@ -24,54 +26,81 @@ func New() Impl {
 
 // Artifact can be used to manage OCI artifacts.
 type Artifact struct {
-	// MountPath is the local path containing the artifact data.
-	MountPath string
-
-	// Cleanup has to be called if the artifact is not used any more.
-	Cleanup func()
+	// Data is the actual artifact content.
+	Data []byte
 }
 
 // defaultImpl is the default implementation for the OCI artifact handling.
 type defaultImpl struct{}
 
-// Pull downloads and mounts the artifact content by using the provided ref.
-func (*defaultImpl) Pull(ctx context.Context, sys *types.SystemContext, ref string) (*Artifact, error) {
-	log.Infof(ctx, "Pulling OCI artifact from ref: %s", ref)
+const (
+	maxArtifactSize = 1024 * 1024 // 1 MiB
+)
+
+// Pull downloads and mounts the artifact content by using the provided image name.
+func (*defaultImpl) Pull(
+	ctx context.Context,
+	sys *types.SystemContext,
+	img string,
+	enforceConfigMediaType string,
+) (*Artifact, error) {
+	log.Infof(ctx, "Pulling OCI artifact from ref: %s", img)
 
-	storeOpts, err := storage.DefaultStoreOptions(false, 0)
+	name, err := libimage.NormalizeName(img)
 	if err != nil {
-		return nil, fmt.Errorf("get default storage options: %w", err)
+		return nil, fmt.Errorf("parse image name: %w", err)
 	}
 
-	store, err := storage.GetStore(storeOpts)
+	ref, err := docker.NewReference(name)
 	if err != nil {
-		return nil, fmt.Errorf("get container storage: %w", err)
+		return nil, fmt.Errorf("create docker reference: %w", err)
 	}
 
-	runtime, err := libimage.RuntimeFromStore(store, &libimage.RuntimeOptions{SystemContext: sys})
+	src, err := ref.NewImageSource(ctx, sys)
 	if err != nil {
-		return nil, fmt.Errorf("create libimage runtime: %w", err)
+		return nil, fmt.Errorf("build image source: %w", err)
 	}
 
-	images, err := runtime.Pull(ctx, ref, config.PullPolicyAlways, &libimage.PullOptions{})
+	manifestBytes, mimeType, err := src.GetManifest(ctx, nil)
 	if err != nil {
-		return nil, fmt.Errorf("pull OCI artifact: %w", err)
+		return nil, fmt.Errorf("get manifest: %w", err)
 	}
-	image := images[0]
 
-	mountPath, err := image.Mount(ctx, nil, "")
+	parsedManifest, err := manifest.FromBlob(manifestBytes, mimeType)
 	if err != nil {
-		return nil, fmt.Errorf("mount OCI artifact: %w", err)
+		return nil, fmt.Errorf("parse manifest: %w", err)
+	}
+	if enforceConfigMediaType != "" && parsedManifest.ConfigInfo().MediaType != enforceConfigMediaType {
+		return nil, fmt.Errorf(
+			"wrong config media type %q, requires %q",
+			parsedManifest.ConfigInfo().MediaType, enforceConfigMediaType,
+		)
+	}
+
+	layers := parsedManifest.LayerInfos()
+	if len(layers) < 1 {
+		return nil, fmt.Errorf("artifact needs at least one layer")
 	}
+	layer := layers[0]
 
-	cleanup := func() {
-		if err := image.Unmount(true); err != nil {
-			log.Warnf(ctx, "Unable to unmount OCI artifact path %s: %v", mountPath, err)
-		}
+	bic := blobinfocache.DefaultCache(sys)
+	rc, size, err := src.GetBlob(ctx, layer.BlobInfo, bic)
+	if err != nil {
+		return nil, fmt.Errorf("get layer blob: %w", err)
+	}
+	defer rc.Close()
+	if size < 0 {
+		return nil, fmt.Errorf("unknown layer blob size")
+	} else if size > maxArtifactSize {
+		return nil, fmt.Errorf("layer exceeds max size of %d bytes", maxArtifactSize)
+	}
+
+	layerBytes, err := io.ReadAll(io.LimitReader(rc, maxArtifactSize))
+	if err != nil {
+		return nil, fmt.Errorf("read from blob: %w", err)
 	}
 
 	return &Artifact{
-		MountPath: mountPath,
-		Cleanup:   cleanup,
+		Data: layerBytes,
 	}, nil
 }
diff --git a/internal/config/seccomp/seccompociartifact/seccompociartifact.go b/internal/config/seccomp/seccompociartifact/seccompociartifact.go
@@ -3,9 +3,6 @@ package seccompociartifact
 import (
 	"context"
 	"fmt"
-	"io/fs"
-	"os"
-	"path/filepath"
 
 	"github.com/containers/image/v5/types"
 
@@ -27,9 +24,14 @@ func New() *SeccompOCIArtifact {
 	}
 }
 
-// SeccompProfilePodAnnotation is the annotation used for matching a whole pod
-// rather than a specific container.
-const SeccompProfilePodAnnotation = annotations.SeccompProfileAnnotation + "/POD"
+const (
+	// SeccompProfilePodAnnotation is the annotation used for matching a whole pod
+	// rather than a specific container.
+	SeccompProfilePodAnnotation = annotations.SeccompProfileAnnotation + "/POD"
+
+	// requiredConfigMediaType is the config media type for OCI artifact seccomp profiles.
+	requiredConfigMediaType = "application/vnd.cncf.seccomp-profile.config.v1+json"
+)
 
 // TryPull tries to pull the OCI artifact seccomp profile while evaluating
 // the provided annotations.
@@ -64,40 +66,11 @@ func (s *SeccompOCIArtifact) TryPull(
 		return nil, nil
 	}
 
-	artifact, err := s.ociArtifactImpl.Pull(ctx, sys, profileRef)
+	artifact, err := s.ociArtifactImpl.Pull(ctx, sys, profileRef, requiredConfigMediaType)
 	if err != nil {
 		return nil, fmt.Errorf("pull OCI artifact: %w", err)
 	}
-	defer artifact.Cleanup()
-
-	const jsonExt = ".json"
-	seccompProfilePath := ""
-	if err := filepath.Walk(artifact.MountPath,
-		func(p string, info os.FileInfo, err error) error {
-			if err != nil {
-				return err
-			}
-
-			if info.IsDir() ||
-				info.Mode()&os.ModeSymlink == os.ModeSymlink ||
-				filepath.Ext(info.Name()) != jsonExt {
-				return nil
-			}
-
-			seccompProfilePath = p
-
-			// TODO(sgrunert): allow merging profiles, not just choosing the first one
-			return fs.SkipAll
-		}); err != nil {
-		return nil, fmt.Errorf("walk %s: %w", artifact.MountPath, err)
-	}
-
-	log.Infof(ctx, "Trying to read profile from: %s", seccompProfilePath)
-	profileContent, err := os.ReadFile(seccompProfilePath)
-	if err != nil {
-		return nil, fmt.Errorf("read %s from file store: %w", seccompProfilePath, err)
-	}
 
-	log.Infof(ctx, "Retrieved OCI artifact seccomp profile of len: %d", len(profileContent))
-	return profileContent, nil
+	log.Infof(ctx, "Retrieved OCI artifact seccomp profile of len: %d", len(artifact.Data))
+	return artifact.Data, nil
 }
diff --git a/internal/config/seccomp/seccompociartifact/seccompociartifact_test.go b/internal/config/seccomp/seccompociartifact/seccompociartifact_test.go
@@ -4,8 +4,6 @@ import (
 	"context"
 	"errors"
 	"io"
-	"os"
-	"path/filepath"
 
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo/v2"
@@ -21,10 +19,7 @@ import (
 // The actual test suite
 var _ = t.Describe("SeccompOCIArtifact", func() {
 	t.Describe("TryPull", func() {
-		const (
-			testProfileContent  = "{}"
-			testSeccompJSONFile = "seccomp.json"
-		)
+		const testProfileContent = "{}"
 
 		var (
 			sut                 *seccompociartifact.SeccompOCIArtifact
@@ -44,13 +39,8 @@ var _ = t.Describe("SeccompOCIArtifact", func() {
 			ociArtifactImplMock = ociartifactmock.NewMockImpl(mockCtrl)
 			sut.SetOCIArtifactImpl(ociArtifactImplMock)
 
-			tempDir, err := os.MkdirTemp("", "seccompociartifact-test-*")
-			Expect(err).NotTo(HaveOccurred())
-			Expect(os.WriteFile(filepath.Join(tempDir, testSeccompJSONFile), []byte(testProfileContent), 0o644)).NotTo(HaveOccurred())
-
 			testArtifact = &ociartifact.Artifact{
-				MountPath: tempDir,
-				Cleanup:   func() { os.RemoveAll(tempDir) },
+				Data: []byte(testProfileContent),
 			}
 		})
 
@@ -71,7 +61,7 @@ var _ = t.Describe("SeccompOCIArtifact", func() {
 		It("should match image specific annotation for whole pod", func() {
 			// Given
 			gomock.InOrder(
-				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
+				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
 			)
 
 			// When
@@ -88,7 +78,7 @@ var _ = t.Describe("SeccompOCIArtifact", func() {
 		It("should match image specific annotation for container", func() {
 			// Given
 			gomock.InOrder(
-				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
+				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
 			)
 
 			// When
@@ -105,7 +95,7 @@ var _ = t.Describe("SeccompOCIArtifact", func() {
 		It("should match pod specific annotation", func() {
 			// Given
 			gomock.InOrder(
-				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
+				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
 			)
 
 			// When
@@ -122,7 +112,7 @@ var _ = t.Describe("SeccompOCIArtifact", func() {
 		It("should match container specific annotation", func() {
 			// Given
 			gomock.InOrder(
-				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
+				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
 			)
 
 			// When
@@ -152,26 +142,8 @@ var _ = t.Describe("SeccompOCIArtifact", func() {
 		It("should fail if artifact pull fails", func() {
 			// Given
 			gomock.InOrder(
-				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errTest),
-			)
-
-			// When
-			res, err := sut.TryPull(context.Background(), nil, "", nil,
-				map[string]string{
-					seccompociartifact.SeccompProfilePodAnnotation: "test",
-				})
-
-			// Then
-			Expect(err).To(HaveOccurred())
-			Expect(res).To(BeNil())
-		})
-
-		It("should fail if seccomp.json is not in artifact", func() {
-			// Given
-			gomock.InOrder(
-				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any()).Return(testArtifact, nil),
+				ociArtifactImplMock.EXPECT().Pull(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errTest),
 			)
-			Expect(os.RemoveAll(filepath.Join(testArtifact.MountPath, testSeccompJSONFile))).NotTo(HaveOccurred())
 
 			// When
 			res, err := sut.TryPull(context.Background(), nil, "", nil,

diff --git a/test/mocks/ociartifact/ociartifact.go b/test/mocks/ociartifact/ociartifact.go
diff --git a/test/seccomp_oci_artifacts.bats b/test/seccomp_oci_artifacts.bats
@@ -16,10 +16,10 @@ function teardown() {
 	cleanup_test
 }
 
-ARTIFACT_IMAGE_WITH_ANNOTATION=quay.io/crio/nginx-seccomp:generic
-ARTIFACT_IMAGE_WITH_POD_ANNOTATION=quay.io/crio/nginx-seccomp:pod
-ARTIFACT_IMAGE_WITH_CONTAINER_ANNOTATION=quay.io/crio/nginx-seccomp:container
-ARTIFACT_IMAGE=quay.io/crio/seccomp:v1
+ARTIFACT_IMAGE_WITH_ANNOTATION=quay.io/crio/nginx-seccomp:v2
+ARTIFACT_IMAGE_WITH_POD_ANNOTATION=$ARTIFACT_IMAGE_WITH_ANNOTATION-pod
+ARTIFACT_IMAGE_WITH_CONTAINER_ANNOTATION=$ARTIFACT_IMAGE_WITH_ANNOTATION-container
+ARTIFACT_IMAGE=quay.io/crio/seccomp:v2
 CONTAINER_NAME=container1
 ANNOTATION=seccomp-profile.kubernetes.cri-o.io
 POD_ANNOTATION=seccomp-profile.kubernetes.cri-o.io/POD
@@ -196,3 +196,16 @@ TEST_SYSCALL=OCI_ARTIFACT_TEST
 	grep -vq "Retrieved OCI artifact seccomp profile" "$CRIO_LOG"
 	crictl inspect "$CTR" | jq -e '.info.runtimeSpec.linux.seccomp == null'
 }
+
+@test "seccomp OCI artifact with missing artifact" {
+	# Run with enabled feature set
+	create_runtime_with_allowed_annotation seccomp $ANNOTATION
+	start_crio
+
+	jq '.annotations += { "'$POD_ANNOTATION'": "wrong" }' \
+		"$TESTDATA"/sandbox_config.json > "$TESTDIR"/sandbox.json
+
+	! crictl run "$TESTDATA/container_config.json" "$TESTDIR/sandbox.json"
+
+	grep -q "try to pull OCI artifact seccomp profile" "$CRIO_LOG"
+}