omaha: Write only SHA256 hash for extra files #629

pothos · 2022-11-30T15:22:02Z

For extra files there is only one hash attribute but it was used for
both a SHA1 and SHA256 entry in the Omaha response. On the wire and in
the UI it's unclear which of the formats it should be.
Use two entries in the database for each hash format and make clear what
the format looks like, i.e., base64 or hex. With a change in go-omaha
we can also omit the empty regular SHA1 hash entry but in any case the
client would be expected to know about the SHA256 attribute and use this
instead. This would match what's done in
https://chromium.googlesource.com/chromium/src.git/+/master/docs/updater/protocol_3_1.md

I've also added a small UI fix and a fix for self-hosting:

frontend: Move pipe out of literal text

The hash tip was rendered as
"Tip: cat update.gz false openssl dgst -sha1 -binary false base64"
where "false" should be "|".
Using "|" does not work in the literal text but it works through templating (which is also nicer for translation).
syncer: Use stored file name when self-hosting

The syncer in self-hosting mode will download the files and store them
with new names. The new name wasn't used for the extra file entries.
Update the extra file entries to use the new name.

How to use

With flatcar/go-omaha#9 the empty attribute would be omitted, which is maybe a nice idea but not required to merge this.

Testing done

Locally with update-engine and with curl.

Also tested that it can sync from the public server (no extra files).
Also tested that a local syncer will download packages correctly from the local nebraska:

extrafile-amd64-2191.5.99-flatcar_production_update.gz.sha256
flatcar-amd64-2191.5.99.gz

Omaha:

          <package name="flatcar-amd64-2191.5.99.gz" hash="r3nufcxgMTZaxYEqL+x2zIoeClk=" size="465881871" required="true"/>
          <package name="extrafile-amd64-2191.5.99-flatcar_production_update.gz.sha256" hash="w/PUx63wqbRs8L8N0kjuGUX8syk=" hash_sha256="2dbeb6478333bd181572ad28163b859197827575f93a5ff524d2381e61a91fc5" size="95" required="false"/>

(flatcar_production_update.gz.sha256 is a dummy extra file, just because it exists already on the server)

backend/pkg/omaha/omaha.go

frontend/src/components/Packages/EditDialog.tsx

t-lo · 2022-12-01T11:59:24Z

Happy with the direction we're taking. Good to go from the conceptual side for me if the implementation gets a LGTM.

t-lo · 2022-12-02T08:54:10Z

@yolossn @joaquimrocha could you please have another look? Are we good to merge?

frontend/src/components/Packages/EditDialog.tsx

The hash tip was rendered as "Tip: cat update.gz false openssl dgst -sha1 -binary false base64" where "false" should be "|". Using "|" does not work in the literal text but it works through templating (which is also nicer for translation).

joaquimrocha

Approving with my last comment which raises still a question about not breaking use-cases.

backend/pkg/omaha/omaha.go

pothos · 2022-12-07T15:33:39Z

Updated it to support both hashes in the extra files.

pothos · 2022-12-07T16:10:31Z

Not sure what's wrong with the test, it wants to apply the migration multiple times
Edit: Worked around it with if exists and if not exists.

pothos · 2022-12-09T12:53:03Z

I've also found a problem with storing extra file names in the syncer-created packages when self-hosting the files.
I tried this but it didn't help:

diff --git a/backend/pkg/syncer/syncer.go b/backend/pkg/syncer/syncer.go
index 9cc6cc7..1892396 100644
--- a/backend/pkg/syncer/syncer.go
+++ b/backend/pkg/syncer/syncer.go
@@ -330,6 +330,7 @@ func (s *Syncer) processUpdate(descriptor channelDescriptor, update *omaha.Updat
                                        logger.Error().Err(err).Str("channel", descriptor.name).Str("arch", descriptor.arch.String()).Msgf("processUpdate, downloading package %s", fileInfo.Name.String)
                                        return err
                                }
+                               fileInfo.Name = null.StringFrom(downloadName)
                        }
                }

Edit: Ah, got it, regular Go foot guns…

diff --git a/backend/pkg/syncer/syncer.go b/backend/pkg/syncer/syncer.go
index 9cc6cc7..6c23963 100644
--- a/backend/pkg/syncer/syncer.go
+++ b/backend/pkg/syncer/syncer.go
@@ -324,12 +324,14 @@ func (s *Syncer) processUpdate(descriptor channelDescriptor, update *omaha.Updat
                                return err
                        }
 
-                       for _, fileInfo := range extraFiles {
+                       for i, _ := range extraFiles {
+                               fileInfo := &extraFiles[i]
                                downloadName := fmt.Sprintf("extrafile-%s-%s-%s", getArchString(descriptor.arch), update.Manifest.Version, fileInfo.Name.String)
                                if err := s.downloadPackage(update, fileInfo.Name.String, fileInfo.Hash.String, fileInfo.Hash256.String, downloadName); err != nil {
                                        logger.Error().Err(err).Str("channel", descriptor.name).Str("arch", descriptor.arch.String()).Msgf("processUpdate, downloading package %s", fileInfo.Name.String)
                                        return err
                                }
+                               fileInfo.Name = null.StringFrom(downloadName)
                        }
                }

pothos · 2022-12-09T12:55:38Z

We also have the problem with go-bindata having disappeared:

env GOBIN=/app/backend/tools/ go install github.com/kevinburke/go-bindata/go-bindata@v3.22.0
#20 58.18 go: github.com/kevinburke/go-bindata/go-bindata@v3.22.0: github.com/kevinburke/go-bindata/go-bindata@v3.22.0: reading https://proxy.golang.org/github.com/kevinburke/go-bindata/go-bindata/@v/v3.22.0.info: 404 Not Found

The github repo is there, not sure if the proxy is broken, I'll try to disable it.

pothos · 2022-12-09T14:01:23Z

CI is green and I have tested it manually in various ways (thus the extra bug fixes), last chance for a final look before merging ;)

illume

I left some questions and notes for your consideration.

Is it possible to provide some steps to test it?

illume · 2022-12-13T09:44:41Z

backend/pkg/api/db/migrations/0018_add_sha256_file_field.sql

@@ -0,0 +1,7 @@
+-- +migrate Up
+
+alter table package_file add column if not exists hash256 varchar(250) default '';


I wonder if this is the best type? It'd expect it to be a set length or NULL?

Reading just the column name, I didn't know what was in there. I looked elsewhere and found out it is hex encoded sha256.

Good idea, we could limit it to 64 - I followed what was done with the other hash, would I need to change the length for the UI?

illume · 2022-12-13T09:51:53Z

backend/pkg/syncer/syncer.go

+	if sha1Base64Checksum != "" && base64.StdEncoding.EncodeToString(hashSha1.Sum(nil)) != sha1Base64Checksum {
+		return errors.New("downloaded file sha1 hash mismatch")
+	}
+	if sha256Base16Checksum != "" && hex.EncodeToString(hashSha256.Sum(nil)) != sha256Base16Checksum {


I'd expect it to use the better sha256 hash if it's there. Or should supplying two hashes be an error?

Any of both may be present and each one should be correct.

backend/pkg/syncer/syncer.go

illume · 2022-12-13T10:14:11Z

backend/pkg/omaha/omaha_test.go

@@ -283,6 +283,7 @@ func TestMultiPackageResponse(t *testing.T) {
 			Name: null.StringFrom("myfile1.txt"),
 			Size: null.StringFrom("1234"),
 			Hash: null.StringFrom("abcd"),
+			Hash256: null.StringFrom("xyz"),


Are the changes to this test sufficient to cover the changes in this PR?

Should be good, below there is one more file entry that doesn't have the new field.

frontend/src/components/Packages/FileList.tsx

illume · 2022-12-13T10:25:35Z

backend/api/spec.yaml

@@ -1260,6 +1260,8 @@ components:
            type: string
          hash:
            type: string
+          hash256:


Is it worth commenting here that it's a hex encoded sha256, and how the two are handled by the API? (Can both be supplied, is hash legacy, and such).

The documentation is in the 3.0 spec
https://github.com/google/omaha/blob/main/doc/ServerProtocolV3.md#package-response (mentioning both fields)
https://github.com/google/omaha/blob/main/doc/ServerProtocol.md#package-element (mentioning hash sha1 base64 encoding
and in the 3.1 spec
https://chromium.googlesource.com/chromium/src.git/+/master/docs/updater/protocol_3_1.md#update-checks-body-update-check-response-objects-update-check-response-9 (mentioning hash sha256 hex encoding but not mentioning the sha1 hash anymore).

The docs don't mention if one is optional but since hash_sha256 was added in a later 3.0 revision I think it's optional. In 3.1 only hash_sha256 is required so I think hash would be optional because it would be an extension.

frontend/src/components/Packages/FileList.tsx

For extra files there is only one hash attribute but it was used for both a SHA1 and SHA256 entry in the Omaha response. On the wire and in the UI it's unclear which of the formats it should be. Use two entries in the database for each hash format and make clear what the format looks like, i.e., base64 or hex. With a change in go-omaha we can also omit the empty regular SHA1 hash entry but in any case the client would be expected to know about the SHA256 attribute and use this instead. This would match what's done in https://chromium.googlesource.com/chromium/src.git/+/master/docs/updater/protocol_3_1.md

The syncer in self-hosting mode will download the files and store them with new names. The new name wasn't used for the extra file entries. Update the extra file entries to use the new name.

The download for go-bindata failed for no reason, it seems there is a problem with the proxy server. Disable the proxy server for now.

There are changes in the go-omaha version that haven't been used yet by Nebraska. One of them is to omit empty hash attributes. Use the latest commit ID in both the backend and the client library.

pothos · 2022-12-13T11:32:23Z

Found one more UI problem with extra files: #634

joaquimrocha

LGTM. I was trying to avoid changes to the DB but after talking to @pothos I realize that maybe older implementations of a legacy installer may still rely on the checksum to be in te sha1 format, so let's have both of them in the DB.

pothos requested review from wrl, t-lo, joaquimrocha and dongsupark November 30, 2022 15:22

pothos force-pushed the kai/ambiguous-hash branch from b8d9e22 to 05c67e5 Compare November 30, 2022 15:48

joaquimrocha requested a review from yolossn November 30, 2022 15:54

joaquimrocha reviewed Nov 30, 2022

View reviewed changes

backend/pkg/omaha/omaha.go Show resolved Hide resolved

frontend/src/components/Packages/EditDialog.tsx Outdated Show resolved Hide resolved

pothos force-pushed the kai/ambiguous-hash branch 4 times, most recently from b7e5655 to 35ec45a Compare December 1, 2022 12:27

t-lo requested a review from joaquimrocha December 2, 2022 08:53

yolossn reviewed Dec 2, 2022

View reviewed changes

frontend/src/components/Packages/EditDialog.tsx Outdated Show resolved Hide resolved

frontend: Move pipe out of literal text

ed0ef7f

The hash tip was rendered as "Tip: cat update.gz false openssl dgst -sha1 -binary false base64" where "false" should be "|". Using "|" does not work in the literal text but it works through templating (which is also nicer for translation).

pothos force-pushed the kai/ambiguous-hash branch from 35ec45a to 53e72ee Compare December 2, 2022 13:46

joaquimrocha approved these changes Dec 6, 2022

View reviewed changes

backend/pkg/omaha/omaha.go Show resolved Hide resolved

pothos force-pushed the kai/ambiguous-hash branch from 53e72ee to 01d7c63 Compare December 7, 2022 15:33

pothos force-pushed the kai/ambiguous-hash branch from 01d7c63 to e9e7027 Compare December 7, 2022 15:54

pothos requested a review from yolossn December 7, 2022 16:10

pothos force-pushed the kai/ambiguous-hash branch from e9e7027 to 69b47eb Compare December 9, 2022 11:30

pothos force-pushed the kai/ambiguous-hash branch from 3b8150c to 9048aa2 Compare December 9, 2022 13:48

pothos requested a review from joaquimrocha December 9, 2022 14:01

illume reviewed Dec 13, 2022

View reviewed changes

frontend/src/components/Packages/FileList.tsx Outdated Show resolved Hide resolved

pothos force-pushed the kai/ambiguous-hash branch 2 times, most recently from 479e47a to 35d47ad Compare December 13, 2022 11:24

pothos added 4 commits December 13, 2022 12:27

syncer: Use stored file name when self-hosting

3367ec2

The syncer in self-hosting mode will download the files and store them with new names. The new name wasn't used for the extra file entries. Update the extra file entries to use the new name.

Dockerfile: For now disable the Go Module Proxy

bedc58c

The download for go-bindata failed for no reason, it seems there is a problem with the proxy server. Disable the proxy server for now.

Use latest go-omaha version

f9eddf3

There are changes in the go-omaha version that haven't been used yet by Nebraska. One of them is to omit empty hash attributes. Use the latest commit ID in both the backend and the client library.

pothos force-pushed the kai/ambiguous-hash branch from 35d47ad to f9eddf3 Compare December 13, 2022 11:27

joaquimrocha approved these changes Dec 13, 2022

View reviewed changes

joaquimrocha merged commit 28bb2f4 into main Dec 13, 2022

joaquimrocha deleted the kai/ambiguous-hash branch December 13, 2022 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

omaha: Write only SHA256 hash for extra files #629

omaha: Write only SHA256 hash for extra files #629

pothos commented Nov 30, 2022 •

edited

Loading

t-lo commented Dec 1, 2022

t-lo commented Dec 2, 2022

joaquimrocha left a comment

pothos commented Dec 7, 2022

pothos commented Dec 7, 2022 •

edited

Loading

pothos commented Dec 9, 2022 •

edited

Loading

pothos commented Dec 9, 2022 •

edited

Loading

pothos commented Dec 9, 2022

illume left a comment

illume Dec 13, 2022

pothos Dec 13, 2022

illume Dec 13, 2022

pothos Dec 13, 2022

illume Dec 13, 2022

pothos Dec 13, 2022

illume Dec 13, 2022

pothos Dec 13, 2022

pothos commented Dec 13, 2022

joaquimrocha left a comment

		@@ -0,0 +1,7 @@
		-- +migrate Up

		alter table package_file add column if not exists hash256 varchar(250) default '';

omaha: Write only SHA256 hash for extra files #629

omaha: Write only SHA256 hash for extra files #629

Conversation

pothos commented Nov 30, 2022 • edited Loading

How to use

Testing done

t-lo commented Dec 1, 2022

t-lo commented Dec 2, 2022

joaquimrocha left a comment

Choose a reason for hiding this comment

pothos commented Dec 7, 2022

pothos commented Dec 7, 2022 • edited Loading

pothos commented Dec 9, 2022 • edited Loading

pothos commented Dec 9, 2022 • edited Loading

pothos commented Dec 9, 2022

illume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pothos commented Dec 13, 2022

joaquimrocha left a comment

Choose a reason for hiding this comment

pothos commented Nov 30, 2022 •

edited

Loading

pothos commented Dec 7, 2022 •

edited

Loading

pothos commented Dec 9, 2022 •

edited

Loading

pothos commented Dec 9, 2022 •

edited

Loading