OCI image-spec / distribution-spec v1.1 updates, first round

copy/single.go

@@ -305,18 +305,18 @@ func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.Syst
 		options := newOrderedSet()
 		match := false
 		for _, wantedPlatform := range wantedPlatforms {
-			// Waiting for https://github.com/opencontainers/image-spec/pull/777 :
-			// This currently can’t use image.MatchesPlatform because we don’t know what to use
-			// for image.Variant.
-			if wantedPlatform.OS == c.OS && wantedPlatform.Architecture == c.Architecture {
+			// For a transitional period, this might trigger warnings because the Variant
+			// field was added to OCI config only recently. If this turns out to be too noisy,
+			// revert this check to only look for (OS, Architecture).
+			if platform.MatchesPlatform(c.Platform, wantedPlatform) {
 				match = true
 				break
 			}
-			options.append(fmt.Sprintf("%s+%s", wantedPlatform.OS, wantedPlatform.Architecture))
+			options.append(fmt.Sprintf("%s+%s+%q", wantedPlatform.OS, wantedPlatform.Architecture, wantedPlatform.Variant))
 		}
 		if !match {
-			logrus.Infof("Image operating system mismatch: image uses OS %q+architecture %q, expecting one of %q",
-				c.OS, c.Architecture, strings.Join(options.list, ", "))
+			logrus.Infof("Image operating system mismatch: image uses OS %q+architecture %q+%q, expecting one of %q",
+				c.OS, c.Architecture, c.Variant, strings.Join(options.list, ", "))
 		}
 	}
 	return nil
@@ -460,8 +460,14 @@ func (ic *imageCopier) copyLayers(ctx context.Context) ([]compressiontypes.Algor
 		encryptAll = len(*ic.c.options.OciEncryptLayers) == 0
 		totalLayers := len(srcInfos)
 		for _, l := range *ic.c.options.OciEncryptLayers {
-			// if layer is negative, it is reverse indexed.
-			layersToEncrypt.Add((totalLayers + l) % totalLayers)
+			switch {
+			case l >= 0 && l < totalLayers:
+				layersToEncrypt.Add(l)
+			case l < 0 && l+totalLayers >= 0: // Implies (l + totalLayers) < totalLayers
+				layersToEncrypt.Add(l + totalLayers) // If l is negative, it is reverse indexed.
+			default:
+				return nil, fmt.Errorf("when choosing layers to encrypt, layer index %d out of range (%d layers exist)", l, totalLayers)
+			}
 		}
 
 		if encryptAll {

docker/docker_client.go

@@ -1,7 +1,6 @@
 package docker
 
 import (
-	"bytes"
 	"context"
 	"crypto/tls"
 	"encoding/json"
@@ -19,6 +18,7 @@ import (
 
 	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/internal/iolimits"
+	"github.com/containers/image/v5/internal/set"
 	"github.com/containers/image/v5/internal/useragent"
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/pkg/docker/config"
@@ -121,6 +121,9 @@ type dockerClient struct {
 	// Private state for detectProperties:
 	detectPropertiesOnce  sync.Once // detectPropertiesOnce is used to execute detectProperties() at most once.
 	detectPropertiesError error     // detectPropertiesError caches the initial error.
+	// Private state for logResponseWarnings
+	reportedWarningsLock sync.Mutex
+	reportedWarnings     *set.Set[string]
 }
 
 type authScope struct {
@@ -281,10 +284,11 @@ func newDockerClient(sys *types.SystemContext, registry, reference string) (*doc
 	}
 
 	return &dockerClient{
-		sys:             sys,
-		registry:        registry,
-		userAgent:       userAgent,
-		tlsClientConfig: tlsClientConfig,
+		sys:              sys,
+		registry:         registry,
+		userAgent:        userAgent,
+		tlsClientConfig:  tlsClientConfig,
+		reportedWarnings: set.New[string](),
 	}, nil
 }
 
@@ -624,9 +628,76 @@ func (c *dockerClient) makeRequestToResolvedURLOnce(ctx context.Context, method
 	if err != nil {
 		return nil, err
 	}
+	if warnings := res.Header.Values("Warning"); len(warnings) != 0 {
+		c.logResponseWarnings(res, warnings)
+	}
 	return res, nil
 }
 
+// logResponseWarnings logs warningHeaders from res, if any.
+func (c *dockerClient) logResponseWarnings(res *http.Response, warningHeaders []string) {
+	c.reportedWarningsLock.Lock()
+	defer c.reportedWarningsLock.Unlock()
+
+	for _, header := range warningHeaders {
+		warningString := parseRegistryWarningHeader(header)
+		if warningString == "" {
+			logrus.Debugf("Ignored Warning: header from registry: %q", header)
+		} else {
+			if !c.reportedWarnings.Contains(warningString) {
+				c.reportedWarnings.Add(warningString)
+				// Note that reportedWarnings is based only on warningString, so that we don’t
+				// repeat the same warning for every request - but the warning includes the URL;
+				// so it may not be specific to that URL.
+				logrus.Warnf("Warning from registry (first encountered at %q): %q", res.Request.URL.Redacted(), warningString)
+			} else {
+				logrus.Debugf("Repeated warning from registry at %q: %q", res.Request.URL.Redacted(), warningString)
+			}
+		}
+	}
+}
+
+// parseRegistryWarningHeader parses a Warning: header per RFC 7234, limited to the warning
+// values allowed by opencontainers/distribution-spec.
+// It returns the warning string if the header has the expected format, or "" otherwise.
+func parseRegistryWarningHeader(header string) string {
+	const expectedPrefix = `299 - "`
+	const expectedSuffix = `"`
+
+	// warning-value = warn-code SP warn-agent SP warn-text	[ SP warn-date ]
+	// distribution-spec requires warn-code=299, warn-agent="-", warn-date missing
+	if !strings.HasPrefix(header, expectedPrefix) || !strings.HasSuffix(header, expectedSuffix) {
+		return ""
+	}
+	header = header[len(expectedPrefix) : len(header)-len(expectedSuffix)]
+
+	// ”Recipients that process the value of a quoted-string MUST handle a quoted-pair
+	// as if it were replaced by the octet following the backslash.”, so let’s do that…
+	res := strings.Builder{}
+	afterBackslash := false
+	for _, c := range []byte(header) { // []byte because escaping is defined in terms of bytes, not Unicode code points
+		switch {
+		case c == 0x7F || (c < ' ' && c != '\t'):
+			return "" // Control characters are forbidden
+		case afterBackslash:
+			res.WriteByte(c)
+			afterBackslash = false
+		case c == '"':
+			// This terminates the warn-text and warn-date, forbidden by distribution-spec, follows,
+			// or completely invalid input.
+			return ""
+		case c == '\\':
+			afterBackslash = true
+		default:
+			res.WriteByte(c)
+		}
+	}
+	if afterBackslash {
+		return ""
+	}
+	return res.String()
+}
+
 // we're using the challenges from the /v2/ ping response and not the one from the destination
 // URL in this request because:
 //
@@ -1008,9 +1079,10 @@ func isManifestUnknownError(err error) bool {
 	if errors.As(err, &e) && e.ErrorCode() == errcode.ErrorCodeUnknown && e.Message == "Not Found" {
 		return true
 	}
-	// ALSO registry.redhat.io as of October 2022
+	// opencontainers/distribution-spec does not require the errcode.Error payloads to be used,
+	// but specifies that the HTTP status must be 404.
 	var unexpected *unexpectedHTTPResponseError
-	if errors.As(err, &unexpected) && unexpected.StatusCode == http.StatusNotFound && bytes.Contains(unexpected.Response, []byte("Not found")) {
+	if errors.As(err, &unexpected) && unexpected.StatusCode == http.StatusNotFound {
 		return true
 	}
 	return false

docker/docker_client_test.go

@@ -332,6 +332,28 @@ func TestNeedsNoRetry(t *testing.T) {
 	}
 }
 
+func TestParseRegistryWarningHeader(t *testing.T) {
+	for _, c := range []struct{ header, expected string }{
+		{"completely invalid", ""},
+		{`299 - "trivial"`, "trivial"},
+		{`100 - "not-299"`, ""},
+		{`299 localhost "warn-agent set"`, ""},
+		{`299 - "no-terminating-quote`, ""},
+		{"299 - \"\x01 control\"", ""},
+		{"299 - \"\\\x01 escaped control\"", ""},
+		{"299 - \"e\\scaped\"", "escaped"},
+		{"299 - \"non-UTF8 \xA1\xA2\"", "non-UTF8 \xA1\xA2"},
+		{"299 - \"non-UTF8 escaped \\\xA1\\\xA2\"", "non-UTF8 escaped \xA1\xA2"},
+		{"299 - \"UTF8 žluťoučký\"", "UTF8 žluťoučký"},
+		{"299 - \"UTF8 \\\xC5\\\xBEluťoučký\"", "UTF8 žluťoučký"},
+		{`299 - "unterminated`, ""},
+		{`299 - "warning" "some-date"`, ""},
+	} {
+		res := parseRegistryWarningHeader(c.header)
+		assert.Equal(t, c.expected, res, c.header)
+	}
+}
+
 func TestIsManifestUnknownError(t *testing.T) {
 	// Mostly a smoke test; we can add more registries here if they need special handling.
 

docker/docker_image_dest.go

@@ -367,6 +367,11 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 
 		// Sanity checks:
 		if reference.Domain(candidateRepo) != reference.Domain(d.ref.ref) {
+			// OCI distribution spec 1.1 allows mounting blobs without specifying the source repo
+			// (the "from" parameter); in that case we might try to use these candidates as well.
+			//
+			// OTOH that would mean we can’t do the “blobExists” check, and if there is no match
+			// we could get an upload request that we would have to cancel.
 			logrus.Debugf("... Internal error: domain %s does not match destination %s", reference.Domain(candidateRepo), reference.Domain(d.ref.ref))
 			continue
 		}

docker/errors.go

@@ -47,7 +47,12 @@ func httpResponseToError(res *http.Response, context string) error {
 }
 
 // registryHTTPResponseToError creates a Go error from an HTTP error response of a docker/distribution
-// registry
+// registry.
+//
+// WARNING: The OCI distribution spec says
+// “A `4XX` response code from the registry MAY return a body in any format.”; but if it is
+// JSON, it MUST use the errcode.Error structure.
+// So, callers should primarily decide based on HTTP StatusCode, not based on error type here.
 func registryHTTPResponseToError(res *http.Response) error {
 	err := handleErrorResponse(res)
 	// len(errs) == 0 should never be returned by handleErrorResponse; if it does, we don't modify it and let the caller report it as is.

internal/image/docker_schema2_test.go

@@ -22,6 +22,8 @@ import (
 	"golang.org/x/exp/slices"
 )
 
+const commonFixtureConfigDigest = "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f"
+
 func manifestSchema2FromFixture(t *testing.T, src types.ImageSource, fixture string, mustFail bool) genericManifest {
 	manifest, err := os.ReadFile(filepath.Join("fixtures", fixture))
 	require.NoError(t, err)
@@ -39,7 +41,7 @@ func manifestSchema2FromComponentsLikeFixture(configBlob []byte) genericManifest
 	return manifestSchema2FromComponents(manifest.Schema2Descriptor{
 		MediaType: "application/octet-stream",
 		Size:      5940,
-		Digest:    "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f",
+		Digest:    commonFixtureConfigDigest,
 	}, nil, configBlob, []manifest.Schema2Descriptor{
 		{
 			MediaType: "application/vnd.docker.image.rootfs.diff.tar.gzip",
@@ -114,7 +116,7 @@ func TestManifestSchema2ConfigInfo(t *testing.T) {
 	} {
 		assert.Equal(t, types.BlobInfo{
 			Size:      5940,
-			Digest:    "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f",
+			Digest:    commonFixtureConfigDigest,
 			MediaType: "application/octet-stream",
 		}, m.ConfigInfo())
 	}
@@ -123,11 +125,12 @@ func TestManifestSchema2ConfigInfo(t *testing.T) {
 // configBlobImageSource allows testing various GetBlob behaviors in .ConfigBlob()
 type configBlobImageSource struct {
 	mocks.ForbiddenImageSource // We inherit almost all of the methods, which just panic()
+	expectedDigest             digest.Digest
 	f                          func() (io.ReadCloser, int64, error)
 }
 
 func (f configBlobImageSource) GetBlob(ctx context.Context, info types.BlobInfo, _ types.BlobInfoCache) (io.ReadCloser, int64, error) {
-	if info.Digest.String() != "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f" {
+	if info.Digest != f.expectedDigest {
 		panic("Unexpected digest in GetBlob")
 	}
 	return f.f()
@@ -163,7 +166,10 @@ func TestManifestSchema2ConfigBlob(t *testing.T) {
 	} {
 		var src types.ImageSource
 		if c.cbISfn != nil {
-			src = configBlobImageSource{f: c.cbISfn}
+			src = configBlobImageSource{
+				expectedDigest: commonFixtureConfigDigest,
+				f:              c.cbISfn,
+			}
 		} else {
 			src = nil
 		}
@@ -350,6 +356,7 @@ func newSchema2ImageSource(t *testing.T, dockerRef string) *schema2ImageSource {
 
 	return &schema2ImageSource{
 		configBlobImageSource: configBlobImageSource{
+			expectedDigest: commonFixtureConfigDigest,
 			f: func() (io.ReadCloser, int64, error) {
 				return io.NopCloser(bytes.NewReader(realConfigJSON)), int64(len(realConfigJSON)), nil
 			},