Merge pull request #2410 from mtrmac/digest-nonsecurity

Non-security digest.Digest use cleanups

copy/encryption.go

@@ -47,13 +47,17 @@ func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo
 	desc := imgspecv1.Descriptor{
 		Annotations: stream.info.Annotations,
 	}
-	reader, decryptedDigest, err := ocicrypt.DecryptLayer(ic.c.options.OciDecryptConfig, stream.reader, desc, false)
+	// DecryptLayer supposedly returns a digest of the decrypted stream.
+	// In pratice, that value is never set in the current implementation.
+	// And we shouldn’t use it anyway, because it is not trusted: encryption can be made to a public key,
+	// i.e. it doesn’t authenticate the origin of the metadata in any way.
+	reader, _, err := ocicrypt.DecryptLayer(ic.c.options.OciDecryptConfig, stream.reader, desc, false)
 	if err != nil {
 		return nil, fmt.Errorf("decrypting layer %s: %w", srcInfo.Digest, err)
 	}
 
 	stream.reader = reader
-	stream.info.Digest = decryptedDigest
+	stream.info.Digest = ""
 	stream.info.Size = -1
 	maps.DeleteFunc(stream.info.Annotations, func(k string, _ string) bool {
 		return strings.HasPrefix(k, "org.opencontainers.image.enc")

docs/atomic-signature-embedded-json.json

@@ -31,7 +31,8 @@
                     "additionalProperties": false,
                     "properties": {
                         "docker-manifest-digest": {
-                            "type": "string"
+                            "type": "string",
+                            "pattern":"^[a-z0-9]+(?:[.+_-][a-z0-9]+)*:[a-zA-Z0-9=_-]+$"
                         }
                     }
                 },

ostree/ostree_src.go

@@ -439,7 +439,10 @@ func (s *ostreeImageSource) LayerInfosForCopy(ctx context.Context, instanceDiges
 		if err != nil {
 			return nil, err
 		}
-		uncompressedDigest := digest.Digest(uncompressedDigestStr)
+		uncompressedDigest, err := digest.Parse(uncompressedDigestStr)
+		if err != nil {
+			return nil, err
+		}
 		blobInfo := types.BlobInfo{
 			Digest:    uncompressedDigest,
 			Size:      uncompressedSize,

pkg/blobcache/src.go

@@ -116,6 +116,63 @@ func (s *blobCacheSource) GetSignaturesWithFormat(ctx context.Context, instanceD
 	return s.source.GetSignaturesWithFormat(ctx, instanceDigest)
 }
 
+// layerInfoForCopy returns a possibly-updated version of info for LayerInfosForCopy
+func (s *blobCacheSource) layerInfoForCopy(info types.BlobInfo) (types.BlobInfo, error) {
+	var replaceDigestBytes []byte
+	blobFile, err := s.reference.blobPath(info.Digest, false)
+	if err != nil {
+		return types.BlobInfo{}, err
+	}
+	switch s.reference.compress {
+	case types.Compress:
+		replaceDigestBytes, err = os.ReadFile(blobFile + compressedNote)
+	case types.Decompress:
+		replaceDigestBytes, err = os.ReadFile(blobFile + decompressedNote)
+	}
+	if err != nil {
+		return info, nil
+	}
+	replaceDigest, err := digest.Parse(string(replaceDigestBytes))
+	if err != nil {
+		return info, nil
+	}
+	alternate, err := s.reference.blobPath(replaceDigest, false)
+	if err != nil {
+		return types.BlobInfo{}, err
+	}
+	fileInfo, err := os.Stat(alternate)
+	if err != nil {
+		return info, nil
+	}
+
+	switch info.MediaType {
+	case v1.MediaTypeImageLayer, v1.MediaTypeImageLayerGzip:
+		switch s.reference.compress {
+		case types.Compress:
+			info.MediaType = v1.MediaTypeImageLayerGzip
+			info.CompressionAlgorithm = &compression.Gzip
+		case types.Decompress: // FIXME: This should remove zstd:chunked annotations (but those annotations being left with incorrect values should not break pulls)
+			info.MediaType = v1.MediaTypeImageLayer
+			info.CompressionAlgorithm = nil
+		}
+	case manifest.DockerV2SchemaLayerMediaTypeUncompressed, manifest.DockerV2Schema2LayerMediaType:
+		switch s.reference.compress {
+		case types.Compress:
+			info.MediaType = manifest.DockerV2Schema2LayerMediaType
+			info.CompressionAlgorithm = &compression.Gzip
+		case types.Decompress:
+			// nope, not going to suggest anything, it's not allowed by the spec
+			return info, nil
+		}
+	}
+	logrus.Debugf("suggesting cached blob with digest %q, type %q, and compression %v in place of blob with digest %q", replaceDigest.String(), info.MediaType, s.reference.compress, info.Digest.String())
+	info.CompressionOperation = s.reference.compress
+	info.Digest = replaceDigest
+	info.Size = fileInfo.Size()
+	logrus.Debugf("info = %#v", info)
+	return info, nil
+}
+
 func (s *blobCacheSource) LayerInfosForCopy(ctx context.Context, instanceDigest *digest.Digest) ([]types.BlobInfo, error) {
 	signatures, err := s.source.GetSignaturesWithFormat(ctx, instanceDigest)
 	if err != nil {
@@ -138,55 +195,10 @@ func (s *blobCacheSource) LayerInfosForCopy(ctx context.Context, instanceDigest
 	if canReplaceBlobs && s.reference.compress != types.PreserveOriginal {
 		replacedInfos := make([]types.BlobInfo, 0, len(infos))
 		for _, info := range infos {
-			var replaceDigest []byte
-			blobFile, err := s.reference.blobPath(info.Digest, false)
+			info, err = s.layerInfoForCopy(info)
 			if err != nil {
 				return nil, err
 			}
-			var alternate string
-			switch s.reference.compress {
-			case types.Compress:
-				alternate = blobFile + compressedNote
-				replaceDigest, err = os.ReadFile(alternate)
-			case types.Decompress:
-				alternate = blobFile + decompressedNote
-				replaceDigest, err = os.ReadFile(alternate)
-			}
-			if err == nil && digest.Digest(replaceDigest).Validate() == nil {
-				alternate, err = s.reference.blobPath(digest.Digest(replaceDigest), false)
-				if err != nil {
-					return nil, err
-				}
-				fileInfo, err := os.Stat(alternate)
-				if err == nil {
-					switch info.MediaType {
-					case v1.MediaTypeImageLayer, v1.MediaTypeImageLayerGzip:
-						switch s.reference.compress {
-						case types.Compress:
-							info.MediaType = v1.MediaTypeImageLayerGzip
-							info.CompressionAlgorithm = &compression.Gzip
-						case types.Decompress: // FIXME: This should remove zstd:chunked annotations (but those annotations being left with incorrect values should not break pulls)
-							info.MediaType = v1.MediaTypeImageLayer
-							info.CompressionAlgorithm = nil
-						}
-					case manifest.DockerV2SchemaLayerMediaTypeUncompressed, manifest.DockerV2Schema2LayerMediaType:
-						switch s.reference.compress {
-						case types.Compress:
-							info.MediaType = manifest.DockerV2Schema2LayerMediaType
-							info.CompressionAlgorithm = &compression.Gzip
-						case types.Decompress:
-							// nope, not going to suggest anything, it's not allowed by the spec
-							replacedInfos = append(replacedInfos, info)
-							continue
-						}
-					}
-					logrus.Debugf("suggesting cached blob with digest %q, type %q, and compression %v in place of blob with digest %q", string(replaceDigest), info.MediaType, s.reference.compress, info.Digest.String())
-					info.CompressionOperation = s.reference.compress
-					info.Digest = digest.Digest(replaceDigest)
-					info.Size = fileInfo.Size()
-					logrus.Debugf("info = %#v", info)
-				}
-			}
 			replacedInfos = append(replacedInfos, info)
 		}
 		infos = replacedInfos

signature/internal/sigstore_payload.go

@@ -150,7 +150,11 @@ func (s *UntrustedSigstorePayload) strictUnmarshalJSON(data []byte) error {
 	}); err != nil {
 		return err
 	}
-	s.untrustedDockerManifestDigest = digest.Digest(digestString)
+	digestValue, err := digest.Parse(digestString)
+	if err != nil {
+		return NewInvalidSignatureError(fmt.Sprintf(`invalid docker-manifest-digest value %q: %v`, digestString, err))
+	}
+	s.untrustedDockerManifestDigest = digestValue
 
 	return ParanoidUnmarshalJSONObjectExactFields(identity, map[string]any{
 		"docker-reference": &s.untrustedDockerReference,

signature/internal/sigstore_payload_test.go

@@ -43,6 +43,8 @@ func TestNewUntrustedSigstorePayload(t *testing.T) {
 }
 
 func TestUntrustedSigstorePayloadMarshalJSON(t *testing.T) {
+	const testDigest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
 	// Empty string values
 	s := NewUntrustedSigstorePayload("", "_")
 	_, err := s.MarshalJSON()
@@ -61,19 +63,19 @@ func TestUntrustedSigstorePayloadMarshalJSON(t *testing.T) {
 	}{
 		{
 			UntrustedSigstorePayload{
-				untrustedDockerManifestDigest: "digest!@#",
+				untrustedDockerManifestDigest: testDigest,
 				untrustedDockerReference:      "reference#@!",
 				untrustedCreatorID:            &creatorID,
 				untrustedTimestamp:            &timestamp,
 			},
-			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"cosign container image signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":1484683104}}",
+			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"" + testDigest + "\"},\"type\":\"cosign container image signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":1484683104}}",
 		},
 		{
 			UntrustedSigstorePayload{
-				untrustedDockerManifestDigest: "digest!@#",
+				untrustedDockerManifestDigest: testDigest,
 				untrustedDockerReference:      "reference#@!",
 			},
-			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"cosign container image signature\"},\"optional\":{}}",
+			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"" + testDigest + "\"},\"type\":\"cosign container image signature\"},\"optional\":{}}",
 		},
 	} {
 		marshaled, err := c.input.MarshalJSON()
@@ -104,6 +106,8 @@ func assertUnmarshalUntrustedSigstorePayloadFails(t *testing.T, input []byte) {
 }
 
 func TestUntrustedSigstorePayloadUnmarshalJSON(t *testing.T) {
+	const testDigest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
 	// Invalid input. Note that json.Unmarshal is guaranteed to validate input before calling our
 	// UnmarshalJSON implementation; so test that first, then test our error handling for completeness.
 	assertUnmarshalUntrustedSigstorePayloadFails(t, []byte("&"))
@@ -115,7 +119,7 @@ func TestUntrustedSigstorePayloadUnmarshalJSON(t *testing.T) {
 	assertUnmarshalUntrustedSigstorePayloadFails(t, []byte("1"))
 
 	// Start with a valid JSON.
-	validSig := NewUntrustedSigstorePayload("digest!@#", "reference#@!")
+	validSig := NewUntrustedSigstorePayload(testDigest, "reference#@!")
 	validJSON, err := validSig.MarshalJSON()
 	require.NoError(t, err)
 
@@ -158,6 +162,7 @@ func TestUntrustedSigstorePayloadUnmarshalJSON(t *testing.T) {
 		func(v mSA) { x(v, "critical", "image")["unexpected"] = 1 },
 		// Invalid "docker-manifest-digest"
 		func(v mSA) { x(v, "critical", "image")["docker-manifest-digest"] = 1 },
+		func(v mSA) { x(v, "critical", "image")["docker-manifest-digest"] = "sha256:../.." },
 		// Invalid "identity" object
 		func(v mSA) { x(v, "critical")["identity"] = 1 },
 		func(v mSA) { delete(x(v, "critical", "identity"), "docker-reference") },
@@ -188,7 +193,7 @@ func TestUntrustedSigstorePayloadUnmarshalJSON(t *testing.T) {
 
 	// Optional fields can be missing
 	validSig = UntrustedSigstorePayload{
-		untrustedDockerManifestDigest: "digest!@#",
+		untrustedDockerManifestDigest: testDigest,
 		untrustedDockerReference:      "reference#@!",
 		untrustedCreatorID:            nil,
 		untrustedTimestamp:            nil,

signature/simple.go

@@ -173,7 +173,11 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error {
 	}); err != nil {
 		return err
 	}
-	s.untrustedDockerManifestDigest = digest.Digest(digestString)
+	digestValue, err := digest.Parse(digestString)
+	if err != nil {
+		return internal.NewInvalidSignatureError(fmt.Sprintf(`invalid docker-manifest-digest value %q: %v`, digestString, err))
+	}
+	s.untrustedDockerManifestDigest = digestValue
 
 	return internal.ParanoidUnmarshalJSONObjectExactFields(identity, map[string]any{
 		"docker-reference": &s.untrustedDockerReference,

signature/simple_test.go

@@ -29,6 +29,8 @@ func TestNewUntrustedSignature(t *testing.T) {
 }
 
 func TestMarshalJSON(t *testing.T) {
+	const testDigest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
 	// Empty string values
 	s := newUntrustedSignature("", "_")
 	_, err := s.MarshalJSON()
@@ -47,19 +49,19 @@ func TestMarshalJSON(t *testing.T) {
 	}{
 		{
 			untrustedSignature{
-				untrustedDockerManifestDigest: "digest!@#",
+				untrustedDockerManifestDigest: testDigest,
 				untrustedDockerReference:      "reference#@!",
 				untrustedCreatorID:            &creatorID,
 				untrustedTimestamp:            &timestamp,
 			},
-			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"atomic container signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":1484683104}}",
+			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"" + testDigest + "\"},\"type\":\"atomic container signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":1484683104}}",
 		},
 		{
 			untrustedSignature{
-				untrustedDockerManifestDigest: "digest!@#",
+				untrustedDockerManifestDigest: testDigest,
 				untrustedDockerReference:      "reference#@!",
 			},
-			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"atomic container signature\"},\"optional\":{}}",
+			"{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"" + testDigest + "\"},\"type\":\"atomic container signature\"},\"optional\":{}}",
 		},
 	} {
 		marshaled, err := c.input.MarshalJSON()
@@ -114,6 +116,8 @@ func assertUnmarshalUntrustedSignatureFails(t *testing.T, schemaLoader gojsonsch
 }
 
 func TestUnmarshalJSON(t *testing.T) {
+	const testDigest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
 	// NOTE: The schema at schemaPath is NOT authoritative; docs/atomic-signature.json and the code is, rather!
 	// The schemaPath references are not testing that the code follows the behavior declared by the schema,
 	// they are testing that the schema follows the behavior of the code!
@@ -132,7 +136,7 @@ func TestUnmarshalJSON(t *testing.T) {
 	assertUnmarshalUntrustedSignatureFails(t, schemaLoader, []byte("1"))
 
 	// Start with a valid JSON.
-	validSig := newUntrustedSignature("digest!@#", "reference#@!")
+	validSig := newUntrustedSignature(testDigest, "reference#@!")
 	validJSON, err := validSig.MarshalJSON()
 	require.NoError(t, err)
 
@@ -166,6 +170,7 @@ func TestUnmarshalJSON(t *testing.T) {
 		func(v mSA) { x(v, "critical", "image")["unexpected"] = 1 },
 		// Invalid "docker-manifest-digest"
 		func(v mSA) { x(v, "critical", "image")["docker-manifest-digest"] = 1 },
+		func(v mSA) { x(v, "critical", "image")["docker-manifest-digest"] = "sha256:../.." },
 		// Invalid "identity" object
 		func(v mSA) { x(v, "critical")["identity"] = 1 },
 		func(v mSA) { delete(x(v, "critical", "identity"), "docker-reference") },
@@ -196,7 +201,7 @@ func TestUnmarshalJSON(t *testing.T) {
 
 	// Optional fields can be missing
 	validSig = untrustedSignature{
-		untrustedDockerManifestDigest: "digest!@#",
+		untrustedDockerManifestDigest: testDigest,
 		untrustedDockerReference:      "reference#@!",
 		untrustedCreatorID:            nil,
 		untrustedTimestamp:            nil,
@@ -208,6 +213,8 @@ func TestUnmarshalJSON(t *testing.T) {
 }
 
 func TestSign(t *testing.T) {
+	const testDigest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
 	defer mech.Close()
@@ -216,7 +223,7 @@ func TestSign(t *testing.T) {
 		t.Skipf("Signing not supported: %v", err)
 	}
 
-	sig := newUntrustedSignature("digest!@#", "reference#@!")
+	sig := newUntrustedSignature(testDigest, "reference#@!")
 
 	// Successful signing
 	signature, err := sig.sign(mech, TestKeyFingerprint, "")

storage/storage_dest.go

@@ -59,7 +59,7 @@ type storageImageDestination struct {
 	nextTempFileID        atomic.Int32             // A counter that we use for computing filenames to assign to blobs
 	manifest              []byte                   // Manifest contents, temporary
 	manifestDigest        digest.Digest            // Valid if len(manifest) != 0
-	untrustedDiffIDValues []digest.Digest          // From config’s RootFS.DiffIDs, valid if not nil
+	untrustedDiffIDValues []digest.Digest          // From config’s RootFS.DiffIDs (not even validated to be valid digest.Digest!); or nil if not read yet
 	signatures            []byte                   // Signature contents, temporary
 	signatureses          map[digest.Digest][]byte // Instance signature contents, temporary
 	metadata              storageImageMetadata     // Metadata contents being built
@@ -767,7 +767,13 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D
 				logrus.Debugf("Skipping commit for layer %q, manifest not yet available", newLayerID)
 				return nil, nil
 			}
+
 			untrustedUncompressedDigest = d
+			// While the contents of the digest are untrusted, make sure at least the _format_ is valid,
+			// because we are going to write it to durable storage in expectedLayerDiffIDFlag .
+			if err := untrustedUncompressedDigest.Validate(); err != nil {
+				return nil, err
+			}
 		}
 
 		flags := make(map[string]interface{})