Repository: osbuild/osbuild-composer
Pull request #3514 (2023-06-26 14:35:49)

Allow any hashing algorithm in osbuild stage inputs
Helper functions that create stage input objects with references always hard-coded `sha256:` as a prefix/algorithm for the checksum.  This prevents the functions from being used in cases where other algorithms are use, like sha1, which is possible with (perhaps older) RPM repositories.  The inputs in osbuild a number of hashing algorithms and we should be able to generate stages with other prefixes when necessary.

This PR removes the `sha256:` prefix in the helper functions and assumes all arguments to these functions provide the correct prefix.

Fixes rhbz#2215043 (RHELPLAN-159871).

This pull request includes:
- [x] adequate testing for the new functionality or fixed issue

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`

Information regarding our GitLab pipeline (Schutzbot):

CI will not be ran automatically if WIP label is applied or the PR is in DRAFT state, instead only
a link will be provided to the pipeline which can then be triggered manually if desired. To run the
CI automatically either switch the PR to ready or apply WIP+test label.

Outside contributors need manual approval from one of the osbuild-composer maintainers.

Schutzbot will only be triggered if all Tests jobs in GitHub workflow succeed.
-->


achilleas-k: I also did a test locally with a custom repo using the `dummy` rpm we use in integration tests and creating a repo with `createrepo -s sha512` (it wouldn't allow me to create a `sha1` repo on Fedora 38).

Before the fix, grepping through the stage inputs of the manifest in the job store:
```
# jq .args.manifest.pipelines[1].stages[1].inputs.packages.references[].id de5fe558-bb03-4aca-9713-e0a5acdc7009.json  | grep sha512
"sha256:sha512:795ddd246d8e65bb65e3340c27c1c90b937397f3610e7f308fddb5bfc5e5b29903a82b85ca0f2be1903dc982b10037e4f657a1f474465766f4cf7f03d1572ce2"
```
and after
```
# jq .args.manifest.pipelines[1].stages[1].inputs.packages.references[].id c68d6939-6f2e-4cca-89b3-aeb49affce2e.json | grep sha512
"sha512:795ddd246d8e65bb65e3340c27c1c90b937397f3610e7f308fddb5bfc5e5b29903a82b85ca0f2be1903dc982b10037e4f657a1f474465766f4cf7f03d1572ce2"
```
achilleas-k: > Looks good, perhaps we want to assert the comment in `NewFilesInputSourceObjectRef`?

Yeah good idea as a sanity check.

Patch:
diff --git a/internal/osbuild/fdo_stage.go b/internal/osbuild/fdo_stage.go
index e5fa8d09a..6d85586b7 100644
--- a/internal/osbuild/fdo_stage.go
+++ b/internal/osbuild/fdo_stage.go
@@ -15,7 +15,7 @@ func (FDOStageInputs) isStageInputs() {}
 func NewFDOStageForRootCerts(rootCertsData string) *Stage {
 	dataBytes := []byte(rootCertsData)
 	input := NewFilesInput(NewFilesInputSourcePlainRef([]string{
-		fmt.Sprintf("%x", sha256.Sum256(dataBytes)),
+		fmt.Sprintf("sha256:%x", sha256.Sum256(dataBytes)),
 	}))
 
 	return &Stage{
diff --git a/internal/osbuild/files_input.go b/internal/osbuild/files_input.go
index 757d3b1cc..0b613471f 100644
--- a/internal/osbuild/files_input.go
+++ b/internal/osbuild/files_input.go
@@ -3,6 +3,7 @@ package osbuild
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"github.com/osbuild/osbuild-composer/internal/container"
 )
@@ -195,13 +196,14 @@ type FilesInputSourcePlainRef []string
 
 func (*FilesInputSourcePlainRef) isFilesInputRef() {}
 
-// NewFilesInputSourcePlainRef creates a FilesInputSourcePlainRef from a list of sha256sums.
-// The slice items are the SHA256 checksums of files as a hexadecimal string without any prefix (e.g. "sha256:").
-func NewFilesInputSourcePlainRef(sha256Sums []string) FilesInputRef {
-	refs := FilesInputSourcePlainRef{}
-	for _, sha256Sum := range sha256Sums {
-		refs = append(refs, fmt.Sprintf("sha256:%s", sha256Sum))
+// NewFilesInputSourcePlainRef creates a FilesInputSourcePlainRef from a list
+// of checksums. The checksums must be prefixed by the name of the corresponding
+// hashing algorithm followed by a colon (e.g. sha256:, sha1:, etc).
+func NewFilesInputSourcePlainRef(checksums []string) FilesInputRef {
+	for _, cs := range checksums {
+		validateChecksum(cs)
 	}
+	refs := FilesInputSourcePlainRef(checksums)
 	return &refs
 }
 
@@ -235,11 +237,14 @@ type FilesInputSourceArrayRefEntry struct {
 	Options *FilesInputSourceOptions `json:"options,omitempty"`
 }
 
-// NewFilesInputSourceArrayRefEntry creates a FilesInputSourceArrayRefEntry from a sha256sum and metadata.
-// The sha256sum is the SHA256 checksum of the file as a hexadecimal string without any prefix (e.g. "sha256:").
-func NewFilesInputSourceArrayRefEntry(sha256Sum string, metadata FilesInputRefMetadata) FilesInputSourceArrayRefEntry {
+// NewFilesInputSourceArrayRefEntry creates a FilesInputSourceArrayRefEntry
+// from a checksum and metadata. The checksum must be prefixed by the name of
+// the corresponding hashing algorithm followed by a colon (e.g. sha256:,
+// sha1:, etc).
+func NewFilesInputSourceArrayRefEntry(checksum string, metadata FilesInputRefMetadata) FilesInputSourceArrayRefEntry {
+	validateChecksum(checksum)
 	ref := FilesInputSourceArrayRefEntry{
-		ID: fmt.Sprintf("sha256:%s", sha256Sum),
+		ID: checksum,
 	}
 	if metadata != nil {
 		ref.Options = &FilesInputSourceOptions{Metadata: metadata}
@@ -269,12 +274,15 @@ type FilesInputSourceObjectRef map[string]FilesInputSourceOptions
 
 func (*FilesInputSourceObjectRef) isFilesInputRef() {}
 
-// NewFilesInputSourceObjectRef creates a FilesInputSourceObjectRef from a map of sha256sums to metadata
-// The key is the SHA256 checksum of the file as a hexadecimal string without any prefix (e.g. "sha256:").
+// NewFilesInputSourceObjectRef creates a FilesInputSourceObjectRef from a map
+// of checksums to metadata. The checksums must be prefixed by the name of the
+// corresponding hashing algorithm followed by a colon (e.g. sha256:, sha1:,
+// etc).
 func NewFilesInputSourceObjectRef(entries map[string]FilesInputRefMetadata) FilesInputRef {
 	refs := FilesInputSourceObjectRef{}
-	for sha256Sum, metadata := range entries {
-		refs[fmt.Sprintf("sha256:%s", sha256Sum)] = FilesInputSourceOptions{Metadata: metadata}
+	for checksum, metadata := range entries {
+		validateChecksum(checksum)
+		refs[checksum] = FilesInputSourceOptions{Metadata: metadata}
 	}
 	return &refs
 }
@@ -294,3 +302,12 @@ func NewFilesInputForManifestLists(containers []container.Spec) *FilesInput {
 	filesRef := FilesInputSourcePlainRef(refs)
 	return NewFilesInput(&filesRef)
 }
+
+// validateChecksum panics if the given string does not contain an algorithm
+// prefix. It checks this by making sure that the : delimiter exists and is not
+// at the start of the string.
+func validateChecksum(checksum string) {
+	if strings.IndexRune(checksum, ':') == 0 || strings.Count(checksum, ":") != 1 {
+		panic(fmt.Sprintf("invalid checksum format; must contain algorithm prefix: %s", checksum))
+	}
+}
diff --git a/internal/osbuild/fsnode.go b/internal/osbuild/fsnode.go
index dbcac95a7..922cab20b 100644
--- a/internal/osbuild/fsnode.go
+++ b/internal/osbuild/fsnode.go
@@ -31,7 +31,7 @@ func GenFileNodesStages(files []*fsnode.File) []*Stage {
 			RemoveDestination: true,
 		})
 		copyStageInputs[copyStageInputKey] = NewFilesInput(NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-			NewFilesInputSourceArrayRefEntry(fileDataChecksum, nil),
+			NewFilesInputSourceArrayRefEntry(fmt.Sprintf("sha256:%s", fileDataChecksum), nil),
 		}))
 
 		if file.Mode() != nil {
diff --git a/internal/osbuild/ignition_stage.go b/internal/osbuild/ignition_stage.go
index e7d7f5132..a31ea2e53 100644
--- a/internal/osbuild/ignition_stage.go
+++ b/internal/osbuild/ignition_stage.go
@@ -26,7 +26,7 @@ func (IgnitionStageInputInline) isStageInputs() {}
 
 func NewIgnitionInlineInput(embeddedData string) Inputs {
 	input := NewFilesInput(NewFilesInputSourcePlainRef([]string{
-		fmt.Sprintf("%x", sha256.Sum256([]byte(embeddedData))),
+		fmt.Sprintf("sha256:%x", sha256.Sum256([]byte(embeddedData))),
 	}))
 	return &IgnitionStageInputInline{InlineFile: input}
 }
diff --git a/internal/osbuild/rpm_stage.go b/internal/osbuild/rpm_stage.go
index ade1dea08..02bbf9fa7 100644
--- a/internal/osbuild/rpm_stage.go
+++ b/internal/osbuild/rpm_stage.go
@@ -1,8 +1,6 @@
 package osbuild
 
 import (
-	"strings"
-
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 )
 
@@ -128,14 +126,13 @@ func NewRpmStageSourceFilesInputs(specs []rpmmd.PackageSpec) *RPMStageInputs {
 func pkgRefs(specs []rpmmd.PackageSpec) FilesInputRef {
 	refs := make([]FilesInputSourceArrayRefEntry, len(specs))
 	for idx, pkg := range specs {
-		pkgSum := strings.TrimPrefix(pkg.Checksum, "sha256:")
 		var pkgMetadata FilesInputRefMetadata
 		if pkg.CheckGPG {
 			pkgMetadata = &RPMStageReferenceMetadata{
 				CheckGPG: pkg.CheckGPG,
 			}
 		}
-		refs[idx] = NewFilesInputSourceArrayRefEntry(pkgSum, pkgMetadata)
+		refs[idx] = NewFilesInputSourceArrayRefEntry(pkg.Checksum, pkgMetadata)
 	}
 	return NewFilesInputSourceArrayRef(refs)
 }


Test patch:
diff --git a/internal/distro/distro_test.go b/internal/distro/distro_test.go
index 4b17580b6..50d6f58db 100644
--- a/internal/distro/distro_test.go
+++ b/internal/distro/distro_test.go
@@ -155,8 +155,8 @@ func TestImageTypePipelineNames(t *testing.T) {
 					// manifest creation doesn't fail.
 					allPipelines := append(imageType.BuildPipelines(), imageType.PayloadPipelines()...)
 					minimalPackageSet := []rpmmd.PackageSpec{
-						{Name: "kernel"},
-						{Name: "filesystem"},
+						{Name: "kernel", Checksum: "sha256:a0c936696eb7d5ee3192bf53b9d281cecbb40ca9db520de72cb95817ad92ac72"},
+						{Name: "filesystem", Checksum: "sha256:6b4bf18ba28ccbdd49f2716c9f33c9211155ff703fa6c195c78a07bd160da0eb"},
 					}
 
 					packageSets := make(map[string][]rpmmd.PackageSpec, len(allPipelines))
diff --git a/internal/manifest/os_test.go b/internal/manifest/os_test.go
index 3e166a865..b36dd28fb 100644
--- a/internal/manifest/os_test.go
+++ b/internal/manifest/os_test.go
@@ -28,7 +28,7 @@ func NewTestOS() *OS {
 
 	os := NewOS(&manifest, build, platform, repos)
 	packages := []rpmmd.PackageSpec{
-		rpmmd.PackageSpec{Name: "pkg1"},
+		{Name: "pkg1", Checksum: "sha1:c02524e2bd19490f2a7167958f792262754c5f46"},
 	}
 	os.serializeStart(packages, nil, nil)
 
diff --git a/internal/osbuild/copy_stage_test.go b/internal/osbuild/copy_stage_test.go
index 94f5c0d36..f57ce2f14 100644
--- a/internal/osbuild/copy_stage_test.go
+++ b/internal/osbuild/copy_stage_test.go
@@ -46,11 +46,11 @@ func TestNewCopyStage(t *testing.T) {
 }
 
 func TestNewCopyStageSimpleSourcesInputs(t *testing.T) {
-	fileSum := "1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"
+	fileSum := "sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"
 
 	paths := []CopyStagePath{
 		{
-			From: fmt.Sprintf("input://inlinefile/sha256:%x", fileSum),
+			From: fmt.Sprintf("input://inlinefile/%x", fileSum),
 			To:   "tree://etc/inlinefile",
 		},
 	}
diff --git a/internal/osbuild/files_input_test.go b/internal/osbuild/files_input_test.go
index 72f84c8aa..7f637c97f 100644
--- a/internal/osbuild/files_input_test.go
+++ b/internal/osbuild/files_input_test.go
@@ -25,20 +25,20 @@ func TestFilesInput_UnmarshalJSON(t *testing.T) {
 		},
 		{
 			name:    "source-plain-ref",
-			ref:     NewFilesInputSourcePlainRef([]string{"1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"}),
+			ref:     NewFilesInputSourcePlainRef([]string{"sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"}),
 			rawJson: []byte(`{"type":"org.osbuild.files","origin":"org.osbuild.source","references":["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"]}`),
 		},
 		{
 			name: "source-array-ref",
 			ref: NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-				NewFilesInputSourceArrayRefEntry("1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef", nil),
+				NewFilesInputSourceArrayRefEntry("sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef", nil),
 			}),
 			rawJson: []byte(`{"type":"org.osbuild.files","origin":"org.osbuild.source","references":[{"id":"sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"}]}`),
 		},
 		{
 			name: "source-object-ref",
 			ref: NewFilesInputSourceObjectRef(map[string]FilesInputRefMetadata{
-				"1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef": nil,
+				"sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef": nil,
 			}),
 			rawJson: []byte(`{"type":"org.osbuild.files","origin":"org.osbuild.source","references":{"sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef":{}}}`),
 		},
diff --git a/internal/osbuild/fsnode_test.go b/internal/osbuild/fsnode_test.go
index 83ad15482..4f3ecab9f 100644
--- a/internal/osbuild/fsnode_test.go
+++ b/internal/osbuild/fsnode_test.go
@@ -53,7 +53,7 @@ func TestGenFileNodesStages(t *testing.T) {
 					},
 				}, &CopyStageFilesInputs{
 					fmt.Sprintf("file-%x", sha256.Sum256(fileData1)): NewFilesInput(NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("%x", sha256.Sum256(fileData1)), nil),
+						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("sha256:%x", sha256.Sum256(fileData1)), nil),
 					})),
 				}),
 			},
@@ -80,10 +80,10 @@ func TestGenFileNodesStages(t *testing.T) {
 					},
 				}, &CopyStageFilesInputs{
 					fmt.Sprintf("file-%x", sha256.Sum256(fileData1)): NewFilesInput(NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("%x", sha256.Sum256(fileData1)), nil),
+						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("sha256:%x", sha256.Sum256(fileData1)), nil),
 					})),
 					fmt.Sprintf("file-%x", sha256.Sum256(fileData2)): NewFilesInput(NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("%x", sha256.Sum256(fileData2)), nil),
+						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("sha256:%x", sha256.Sum256(fileData2)), nil),
 					})),
 				}),
 			},
@@ -110,10 +110,10 @@ func TestGenFileNodesStages(t *testing.T) {
 					},
 				}, &CopyStageFilesInputs{
 					fmt.Sprintf("file-%x", sha256.Sum256(fileData1)): NewFilesInput(NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("%x", sha256.Sum256(fileData1)), nil),
+						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("sha256:%x", sha256.Sum256(fileData1)), nil),
 					})),
 					fmt.Sprintf("file-%x", sha256.Sum256(fileData2)): NewFilesInput(NewFilesInputSourceArrayRef([]FilesInputSourceArrayRefEntry{
-						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("%x", sha256.Sum256(fileData2)), nil),
+						NewFilesInputSourceArrayRefEntry(fmt.Sprintf("sha256:%x", sha256.Sum256(fileData2)), nil),
 					})),
 				}),
 				NewChmodStage(&ChmodStageOptions{
diff --git a/internal/osbuild/rpm_stage_test.go b/internal/osbuild/rpm_stage_test.go
index 4879d3762..0ded7646b 100644
--- a/internal/osbuild/rpm_stage_test.go
+++ b/internal/osbuild/rpm_stage_test.go
@@ -87,3 +87,96 @@ func Test_OSBuildMetadataToRPMs(t *testing.T) {
 	// if neither GPG nor PGP is set, the signature is nil
 	require.Nil(t, rpms[2].Signature)
 }
+
+func TestNewRpmStageSourceFilesInputs(t *testing.T) {
+
+	assert := assert.New(t)
+	require := require.New(t)
+
+	pkgSpecs := []rpmmd.PackageSpec{
+		{
+			Name:           "openssl-libs",
+			Epoch:          1,
+			Version:        "3.0.1",
+			Release:        "5.el9",
+			Arch:           "x86_64",
+			RemoteLocation: "https://example.com/repo/Packages/openssl-libs-3.0.1-5.el9.x86_64.rpm",
+			Checksum:       "sha256:fcf2515ec9115551c99d552da721803ecbca23b7ae5a974309975000e8bef666",
+			Secrets:        "",
+			CheckGPG:       false,
+			IgnoreSSL:      true,
+		},
+		{
+			Name:           "openssl-pkcs11",
+			Epoch:          0,
+			Version:        "0.4.11",
+			Release:        "7.el9",
+			Arch:           "x86_64",
+			RemoteLocation: "https://example.com/repo/Packages/openssl-pkcs11-0.4.11-7.el9.x86_64.rpm",
+			Checksum:       "sha256:4be41142a5fb2b4cd6d812e126838cffa57b7c84e5a79d65f66bb9cf1d2830a3",
+			Secrets:        "",
+			CheckGPG:       false,
+			IgnoreSSL:      true,
+		},
+		{
+			Name:           "p11-kit",
+			Epoch:          0,
+			Version:        "0.24.1",
+			Release:        "2.el9",
+			Arch:           "x86_64",
+			RemoteLocation: "https://example.com/repo/Packages/p11-kit-0.24.1-2.el9.x86_64.rpm",
+			Checksum:       "sha256:da167e41efd19cf25fd1c708b6f123d0203824324b14dd32401d49f2aa0ef0a6",
+			Secrets:        "",
+			CheckGPG:       false,
+			IgnoreSSL:      true,
+		},
+		{
+			Name:           "package-with-sha1-checksum",
+			Epoch:          1,
+			Version:        "3.4.2.",
+			Release:        "10.el9",
+			Arch:           "x86_64",
+			RemoteLocation: "https://example.com/repo/Packages/package-with-sha1-checksum-4.3.2-10.el9.x86_64.rpm",
+			Checksum:       "sha1:6e01b8076a2ab729d564048bf2e3a97c7ac83c13",
+			Secrets:        "",
+			CheckGPG:       true,
+			IgnoreSSL:      true,
+		},
+		{
+			Name:           "package-with-md5-checksum",
+			Epoch:          1,
+			Version:        "3.4.2.",
+			Release:        "5.el9",
+			Arch:           "x86_64",
+			RemoteLocation: "https://example.com/repo/Packages/package-with-md5-checksum-4.3.2-5.el9.x86_64.rpm",
+			Checksum:       "md5:8133f479f38118c5f9facfe2a2d9a071",
+			Secrets:        "",
+			CheckGPG:       true,
+			IgnoreSSL:      true,
+		},
+	}
+	inputs := NewRpmStageSourceFilesInputs(pkgSpecs)
+
+	refsArrayPtr, convOk := inputs.Packages.References.(*FilesInputSourceArrayRef)
+	require.True(convOk)
+	require.NotNil(refsArrayPtr)
+
+	refsArray := *refsArrayPtr
+
+	for idx := range refsArray {
+		refItem := refsArray[idx]
+		pkg := pkgSpecs[idx]
+		assert.Equal(pkg.Checksum, refItem.ID)
+
+		if pkg.CheckGPG {
+			// GPG check enabled: metadata expected
+			require.NotNil(refItem.Options)
+			require.NotNil(refItem.Options.Metadata)
+
+			md, convOk := refItem.Options.Metadata.(*RPMStageReferenceMetadata)
+			require.True(convOk)
+			require.NotNil(md)
+			assert.Equal(md.CheckGPG, pkg.CheckGPG)
+		}
+	}
+}