sha256 sums for OVAs + download integrity

[miabbott]

(filing this issue here, since it crosses a couple of repos)

We made the decision back in 2019 to not compress OVA artifacts as part of cosa compress

One of the side effects of this was that the meta.json doesn't have an uncompressed-sha256 entry for the OVA:

$ cat meta.json | jq .images.vmware
{
  "path": "rhcos-411.85.202203181601-0-vmware.x86_64.ova",
  "sha256": "a39054725aea870a929046abcdfd897e61763e2556844e47bb87492c4367cb74",
  "size": 1118535680,
  "skip-compression": true
}

Consequently, the stream metadata for the RHCOS builds are also missing this entry:

$ curl -Ls https://raw.githubusercontent.com/openshift/installer/master/data/data/coreos/rhcos.json | jq .architectures.x86_64.artifacts.vmware
{
  "release": "410.84.202201251210-0",
  "formats": {
    "ova": {
      "disk": {
        "location": "https://rhcos-redirector.apps.art.xq1c.p1.openshiftapps.com/art/storage/releases/rhcos-4.10/410.84.202201251210-0/x86_64/rhcos-410.84.202201251210-0-vmware.x86_64.ova",
        "sha256": "3d75bcf4d1245f1d10865072b3a735333f2fbc7262b55fafc0fbead8c8c3517d"
      }
    }
  }
}

Which in turn means that the openshift/installer code does not append any sha256 sum when generating URIs for the OVAs:

https://github.com/openshift/installer/blob/6d778f911e79afad8ba2ff4301eda5b5cf4d8e9e/pkg/rhcos/builds.go#L50-L63

I believe this may contribute to problems like https://bugzilla.redhat.com/show_bug.cgi?id=2065849 where the download is not verified and then causes problems when vSphere tries to ingest the OVA.

So this is kind of a broad issue to discuss if there are improvements/changes we should consider for the OVA case.

• Perhaps we duplicate the sha256 entry for OVAs as uncompressed-sha256?
• Perhaps we remove the sha256 entry and replace it with uncompressed-sha256?
• Perhaps we modify the installer code to use the sha256 entry if there is no uncompressed-sha256 entry?

[jlebon]

This is also the case for other uncompressed artifacts, like the live ISO and PXE bits. Hmm, another alternative is to have the installer append both SHAs. Then it's up to consumers of that URL whether to verify before decompressing, after decompressing, or even both.

[cgwalters]

This also relates to coreos/fedora-coreos-tracker#773 - size validation would also immediately show an error except in actively malicious cases.

[cgwalters]

Remember there are two things here: meta.json and stream metadata. As of recent OCP all we care about in openshift-installer is the latter actually. (IOW it's the same for both FCOS/RHCOS)

I think the fix is to openshift-installer - it should honor sha256 regardless, and also check the uncompressed version if it exists.

Something like:

diff --git a/pkg/rhcos/builds.go b/pkg/rhcos/builds.go
index 5dcd1c5eb..15d0a5a89 100644
--- a/pkg/rhcos/builds.go
+++ b/pkg/rhcos/builds.go
@@ -47,9 +47,8 @@ func FetchCoreOSBuild(ctx context.Context) (*stream.Stream, error) {
 	return &st, nil
 }
 
-// FormatURLWithIntegrity squashes an artifact into a URL string
-// with the uncompressed sha256 as a query parameter.  This is necessary
-// currently because various parts of the installer pass around this
+// FormatURLWithIntegrity squashes an artifact into a URL string.
+// This is necessary currently because various parts of the installer pass around this
 // reference as a string, and it's also exposed to users via install-config overrides.
 func FormatURLWithIntegrity(artifact *stream.Artifact) (string, error) {
 	u, err := url.Parse(artifact.Location)
@@ -57,7 +56,11 @@ func FormatURLWithIntegrity(artifact *stream.Artifact) (string, error) {
 		return "", fmt.Errorf("failed to parse artifact URL: %v", err)
 	}
 	q := u.Query()
+	// For historical reasons, openshift-installer uses "sha256" to mean
+	// uncompressed digest.  The CoreOS stream metadata default is
+	// the compressed version.
 	q.Set("sha256", artifact.UncompressedSha256)
+	q.Set("csha256", artifact.Sha256)
 	u.RawQuery = q.Encode()
 	return u.String(), nil
 }
diff --git a/pkg/tfvars/internal/cache/cache.go b/pkg/tfvars/internal/cache/cache.go
index 35b56f3d1..f35230319 100644
--- a/pkg/tfvars/internal/cache/cache.go
+++ b/pkg/tfvars/internal/cache/cache.go
@@ -56,7 +56,7 @@ func getCacheDir(dataType string) (string, error) {
 }
 
 // cacheFile puts data in the cache
-func cacheFile(reader io.Reader, filePath string, sha256Checksum string) (err error) {
+func cacheFile(reader io.Reader, filePath string, compressedSha256Expected, uncompressedSha256Expected string) (err error) {
 	logrus.Debugf("Unpacking file into %q...", filePath)
 
 	flockPath := fmt.Sprintf("%s.lock", filePath)
@@ -143,7 +143,7 @@ func cacheFile(reader io.Reader, filePath string, sha256Checksum string) (err er
 
 	// Wrap the reader in TeeReader to calculate sha256 checksum on the fly
 	hasher := sha256.New()
-	if sha256Checksum != "" {
+	if uncompressedSha256Expected != "" {
 		reader = io.TeeReader(reader, hasher)
 	}
 
@@ -159,11 +159,11 @@ func cacheFile(reader io.Reader, filePath string, sha256Checksum string) (err er
 	closed = true
 
 	// Validate sha256 checksum
-	if sha256Checksum != "" {
+	if uncompressedSha256Expected != "" {
 		foundChecksum := fmt.Sprintf("%x", hasher.Sum(nil))
-		if sha256Checksum != foundChecksum {
+		if uncompressedSha256Expected != foundChecksum {
 			logrus.Error("File sha256 checksum is invalid.")
-			return errors.Errorf("Checksum mismatch for %s; expected=%s found=%s", filePath, sha256Checksum, foundChecksum)
+			return errors.Errorf("Checksum mismatch for %s; expected=%s found=%s", filePath, uncompressedSha256Expected, foundChecksum)
 		}
 
 		logrus.Debug("Checksum validation is complete...")
@@ -172,11 +172,14 @@ func cacheFile(reader io.Reader, filePath string, sha256Checksum string) (err er
 	return os.Rename(tempPath, filePath)
 }
 
-// urlWithIntegrity pairs a URL with an optional expected sha256 checksum (after decompression, if any)
-// If the query string contains sha256 parameter (i.e. https://example.com/data.bin?sha256=098a5a...),
-// then the downloaded data checksum will be compared with the provided value.
+// urlWithIntegrity pairs a URL with two optional expected sha256 checksums; one
+// before decompression, and one after (if any compression applies).
+// If the query string contains a non-empty parameter (i.e. https://example.com/data.bin?sha256=098a5a...),
+// then the downloaded data will be compared with the provided checksums.
+// If both are specified, both will be used.
 type urlWithIntegrity struct {
 	location           url.URL
+	sha256             string
 	uncompressedSHA256 string
 }
 

to start

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[bgilbert]

/remove-lifecycle stale
/lifecycle frozen