Lock the digests by adding sha256 values for installed packages. #301

nmahendru · 2022-08-11T01:02:29Z

Populate sha256sums in a specified manifest file
Usage:
hermit manifest add-digests <manifest.hcl>

It should populate(in place) all the sha256sums for all the version/platform combinations in the original manifest file.

Testing:

An integration test has been added to verify the digest created.
An integration test has been added to verify that hermit install fails
on wrong digest.

alecthomas

Thanks for this, but it needs some reworking. Please let me know if you'd like to pair on it.

app/manifest_lock_digests_cmd.go

alecthomas · 2022-09-26T23:19:12Z

app/manifest_lock_digests_cmd.go

+		return errors.WithStack(err)
+	}
+	if len(installed) == 0 {
+		fmt.Printf("There are no packages installed. Nothing to do here.")


Don't use Printf directly, use ui.UI.Infof() and friends.

app/manifest_lock_digests_cmd.go

env.go

app/manifest_lock_digests_cmd.go

alecthomas · 2022-09-27T01:24:18Z

app/manifest_lock_digests_cmd.go

+		value, _ := hcl.Marshal(localmanifest.Manifest)
+
+		//Now just write down fresh manifest files in a subdirectory in the `bin`.
+		os.MkdirAll(filepath.Join(env.BinDir(), "lockedDigests"), os.ModePerm)


Rename lockedDigests to hermit.lock.

Also, check for errors here and elsewhere. You can use errors.Is(err, fs.ErrExist) to ignore "already exists" errors.

cache/git.go

quad

nit: this PR introduces three unique terms for the same concept:

"digests" / "Digests"
sha256sums
checksums / "actual checksums"

Please consider picking and using one term.

(I prefer "digests"; "sha256sums" is implementation specific and "checksums" is inaccurate.)

quad · 2022-09-26T23:07:10Z

manifest/autoversion/autoversion.go

@@ -79,6 +79,7 @@ func AutoVersion(httpClient *http.Client, ghClient GitHubClient, path string) (l
 // "hclBlock" is the block containing the auto-version information. Its "Labels" field should be updated with the new versions, if any.
 func parseVersionBlockFromManifest(manifest []byte) (ast *hcl.AST, hclBlock *hcl.Block, autoVersionBlock *hmanifest.AutoVersionBlock, err error) {
 	ast, err = hcl.ParseBytes(manifest)
+


Suggested change

quad · 2022-09-27T13:19:03Z

env.go

@@ -675,7 +675,7 @@ func (e *Env) Upgrade(l *ui.UI, pkg *manifest.Package) (*shell.Changes, error) {
 //
 // Link chains are in the form
 //
-//     <binary> -> <pkg>-<version>.pkg -> hermit
+//	<binary> -> <pkg>-<version>.pkg -> hermit


nit: was this intentional?

manifest/resolver.go

quad · 2022-09-27T13:20:03Z

state/state.go

@@ -290,7 +292,6 @@ func (s *State) CacheAndUnpack(b *ui.Task, p *manifest.Package) error {
 		return errors.WithStack(err)
 	}
 	defer lock.Release(b)
-


Suggested change

nit: intentional removal?

quad · 2022-09-27T13:22:42Z

app/manifest_lock_digests_cmd.go

+	`
+}
+func (e *lockDigestsCmd) Run(l *ui.UI, env *hermit.Env, cache *cache.Cache, state *state.State) error {
+	//time.Sleep(time.Second * 7)


Suggested change

//time.Sleep(time.Second * 7)

quad · 2022-09-27T13:41:27Z

Out of curiosity, why a unique .hcl file per package vs. a single hermit-lock.hcl?

quad · 2022-09-27T13:25:24Z

app/commands.go

+	Exec        execCmd              `cmd:"" help:"Directly execute a binary in a package." group:"env"`
+	Env         envCmd               `cmd:"" help:"Manage environment variables." group:"env"`
+	Validate    activatedValidateCmd `cmd:"" help:"Hermit validation." group:"global"`
+	LockDigests lockDigestsCmd       `cmd:"" help:"Lock the Digests for installed packages using sha256sums." group:"global"`


Suggested change

LockDigests lockDigestsCmd `cmd:"" help:"Lock the Digests for installed packages using sha256sums." group:"global"`

LockDigests lockDigestsCmd `cmd:"" help:"Lock the manifest for installed packages." group:"global"`

I think we are really locking in the digests and not the manifest. also again sha256sums is an existing attribute so that information is helpful.

I'm not particularly fussed about the verbage; more that they should they be the same.

quad · 2022-09-27T13:26:12Z

app/manifest_cmd.go

@@ -4,4 +4,5 @@ type manifestCmd struct {
 	Validate    validateSourceCmd `cmd:"" help:"Check a package manifest source for errors." group:"global"`
 	AutoVersion autoVersionCmd    `cmd:"" help:"Upgrade manifest versions automatically where possible." group:"global"`
 	Create      manifestCreateCmd `cmd:"" help:"Create a new manifest from an existing package artefact URL." group:"global"`
+	LockDigests lockDigestsCmd    `cmd:"" help:"Update the manifest files for installed packages with sha256sums" group:"global"`


Suggested change

LockDigests lockDigestsCmd `cmd:"" help:"Update the manifest files for installed packages with sha256sums" group:"global"`

LockDigests lockDigestsCmd `cmd:"" help:"Lock the manifest for installed packages." group:"global"`

same as above

quad · 2022-09-27T13:26:55Z

app/manifest_lock_digests_cmd.go

+
+func (e *lockDigestsCmd) Help() string {
+	return `
+	This command will build a list of installed packages and their checksums(SHA256) values.


Suggested change

This command will build a list of installed packages and their checksums(SHA256) values.

This command will build a list of installed packages and their digests (SHA256).

quad · 2022-09-27T13:27:06Z

app/manifest_lock_digests_cmd.go

+
+	That file can be manually reviewed by the project owner and checked in to the repo.
+
+	On subsequent uses of the hermit environment, the installed dependencies will have a reference checksum


Suggested change

On subsequent uses of the hermit environment, the installed dependencies will have a reference checksum

On subsequent uses of the hermit environment, the installed dependencies will have a reference digest

app/manifest_lock_digests_cmd.go

quad · 2022-09-27T13:31:02Z

app/manifest_lock_digests_cmd.go

+		for _, mc := range localmanifest.Manifest.Versions {
+			for _, p := range platform.Core {
+				ref = manifest.ParseReference(ref.Name + "-" + mc.Version[0])


Suggested change

for _, mc := range localmanifest.Manifest.Versions {

for _, p := range platform.Core {

ref = manifest.ParseReference(ref.Name + "-" + mc.Version[0])

for _, mc := range localmanifest.Manifest.Versions {

ref = manifest.ParseReference(ref.Name + "-" + mc.Version[0])

for _, p := range platform.Core {

good optimization. Done!

quad · 2022-09-27T13:32:13Z

app/manifest_lock_digests_cmd.go

+				if err != nil {
+					l.Debugf("Continuing with the next platform tuple.  Current %s: %s", p.OS, p.Arch)
+					continue
+				}


nit: we don't want to log the error?

Every package might not have every platform combo. We try to digest what we can resolve. Refer:
#301 (comment)

app/manifest_lock_digests_cmd.go

quad · 2022-09-27T13:38:06Z

app/manifest_lock_digests_cmd.go

+				if p.OS == "Darwin" {
+					config = manifest.DarwinConfig(res.GetConfig(), p.Arch)
+				} else {
+					config = manifest.LinuxConfig(res.GetConfig())
+				}


Is the OS value always passed through?

How does this handle digests for multiple architectures? (e.g. Linux / arm64)

Is the OS value always passed through?

yes. But it also helps decide what to call, Linux or DarwinConfig

How does this handle digests for multiple architectures? (e.g. Linux / arm64)

the if here is handling linux/mac and there is one check inside DarwinConfig that handles arm/amd64

Both DarwinConfig and LinuxConfig leave the OS` value unchanged.

What about Linux / arm64? It seems LinuxConfig ignores that combination?

I am going by:
https://github.com/cashapp/hermit/blob/master/platform/platform.go#L27-L34

AIUI that's the minimum. @alecthomas?

nmahendru · 2022-09-27T13:45:30Z

nit: this PR introduces three unique terms for the same concept:

"digests" / "Digests"

sha256sums

checksums / "actual checksums"

Please consider picking and using one term.

(I prefer "digests"; "sha256sums" is implementation specific and "checksums" is inaccurate.)
Good point.

sha256sums is an existing term(part of the manifest). I have renamed checksum to digest in the new command. Thinking more about it, I see that the terminology checksum was present prior and that's what I think prompted me to use it where I saw it.

quad · 2022-09-27T13:49:45Z

state/state.go

+		}
+		actualDigest = ch
+	} else {
+		path := s.cache.Path(p.SHA256, p.Source)


Is p.SHA256 already the sha256 value? Can we use that instead of re-calculating?

AFAIK, At the moment manifest files have sha256 and now this command is called locked-digests so these are the only two.

Then would it be more consistent to use "sha256" everywhere, not introducing the concepts of "checksum" or "digest"?

I have removed checksum. sha256 in the manifest file tells the user about what algorithm is used and that is essential information. lock-digests is a generic enough name that if we later want to use sha512 then it would still be applicable. wdyt ?

I'm still not clear why we can't use p.SHA256?

I think what @quad is suggesting is that if p.SHA256 != "", just return that immediately.

Those are all good points. AFAIK the p.SHA256 value is coming from a user and it was manually calculated using maybe something like:

sha256sum <file>

I am thinking if we should check the value here at least ?
The reason I am with that is we plan to use this tooling to automatically update digests in our sources so we should be trusting this more than user inputs ?

made the change and skipping calculation if already present.

state/state.go

quad · 2022-09-27T13:59:26Z

Good point.

sha256sums is an existing term(part of the manifest). I have renamed checksum to digest in the new command. Thinking more about it, I see that the terminology checksum was present prior and that's what I think prompted me to use it where I saw it.

Some concepts are in the code and some concepts are shown to the user.

Which ones are already shown to the user?

nmahendru · 2022-09-27T14:01:43Z

Out of curiosity, why a unique .hcl file per package vs. a single hermit-lock.hcl?

So that we can just put it inside hermit-packages alongside every file. The existing checks will just work transparently then.

nmahendru · 2022-09-27T14:02:43Z

Good point.
sha256sums is an existing term(part of the manifest). I have renamed checksum to digest in the new command. Thinking more about it, I see that the terminology checksum was present prior and that's what I think prompted me to use it where I saw it.

Some concepts are in the code and some concepts are shown to the user.

Which ones are already shown to the user?

AFAIK, At the moment manifest files have sha256 and now this command is called locked-digests so these are the only two.

quad · 2022-09-27T14:05:53Z

AFAIK, At the moment manifest files have sha256 and now this command is called locked-digests so these are the only two.

Then would it be more consistent to use "sha256" everywhere, not introducing the concepts of "checksum" or "digest"?

quad · 2022-09-27T16:01:04Z

cache/http.go

@@ -82,17 +83,17 @@ func (s *httpSource) Validate() error {
 	return nil
 }

-func downloadHTTP(b *ui.Task, response *http.Response, checksum string, uri string, cachePath string) (path string, etag string, err error) {
+func downloadHTTP(b *ui.Task, response *http.Response, checksum string, uri string, cachePath string, cache *Cache) (path string, etag string, returnChecksum string, err error) {


Where is cache used?

state/state.go

quad

This is soooo nice

quad · 2022-09-29T04:48:53Z

manifest/resolver.go

+func NewPackage(manifest *AnnotatedManifest, platform2 platform.Platform, ref Reference) (*Package, error) {
+	config := Config{}
+	config.Platform = platform2


platform2?

This is goland doing it for me and I did not bother :)

quad · 2022-09-29T04:52:37Z

cache/source.go

+	// If the file is a directory then no checksum is required
+	if info.IsDir() {


Suggested change

// If the file is a directory then no checksum is required

if info.IsDir() {

// If the file isn't a regular file then no checksum is required

if !info.IsRegular() {

quad · 2022-09-29T04:53:55Z

state/state.go

+		}
+		actualDigest = ch
+	} else {
+		path := s.cache.Path(p.SHA256, p.Source)


I'm still not clear why we can't use p.SHA256?

alecthomas

Overall LGTM!

alecthomas · 2022-09-29T12:12:50Z

app/manifest_add_digests_cmd.go

+		}
+		err = os.WriteFile(f, value, os.ModePerm)
+		if err != nil {
+			l.Errorf("Could not write the manifest file %s", f)


This should be returned in the error below using errors.Wrapf()

alecthomas · 2022-09-29T12:15:48Z

cache/source.go

+		return s.path, "", "", nil
+	}
+	var data []byte
+	data, err = os.ReadFile(s.path)


This is almost never a good idea with arbitrary inputs. Instead, do:

r, err := os.Open(s.path) h := sha256.New() _, err = io.Copy(h, r)

alecthomas · 2022-09-29T12:16:15Z

it/full/spec/it_spec.sh

@@ -255,6 +255,23 @@ Describe "Hermit"
    End
  End

+  Describe "Running the add-digests command"


alecthomas · 2022-09-29T12:20:11Z

state/state.go

+	} else {
+		// if the artifact is cached then just calculate the digest.
+		path := s.cache.Path(p.SHA256, p.Source)
+		data, err := os.ReadFile(path)


As above, change this to use os.Open() + io.Copy()

alecthomas · 2022-09-29T12:22:10Z

state/state.go

+		}
+		actualDigest = ch
+	} else {
+		path := s.cache.Path(p.SHA256, p.Source)


I think what @quad is suggesting is that if p.SHA256 != "", just return that immediately.

Populate sha256sums in a specified manifest file Usage: hermit manifest add-digests <manifest.hcl> It should populate(in place) all the sha256sums for all the version/platform combinations in the original manifest file. Testing: An integration test has been added to verify the digest created. An integration test has been added to verify that hermit install fails on wrong digest.

1. Removed duplicate code for manifest parsing. 2. Iteration on platform.core instead of manually indexing all possible combinations. 3. Logging improvements. 4. optimizations.

Renaming checksum to digests. and some formatting cleanups.

Also reduced some duplication for digest calculation.

alecthomas · 2022-09-30T00:02:16Z

Thanks!

nmahendru force-pushed the nmahendru/lock_digests_embedded branch 2 times, most recently from 8e46bf1 to 394f460 Compare September 26, 2022 22:56

nmahendru marked this pull request as ready for review September 26, 2022 22:56

alecthomas requested changes Sep 27, 2022

View reviewed changes

nmahendru requested a review from alecthomas September 27, 2022 11:47

nmahendru force-pushed the nmahendru/lock_digests_embedded branch from b334bf7 to 61de0eb Compare September 27, 2022 11:58

quad reviewed Sep 27, 2022

View reviewed changes

nmahendru force-pushed the nmahendru/lock_digests_embedded branch from d92d216 to 09e5173 Compare September 29, 2022 04:27

quad approved these changes Sep 29, 2022

View reviewed changes

alecthomas approved these changes Sep 29, 2022

View reviewed changes

nmahendru and others added 11 commits September 29, 2022 15:39

PR feedback.

23d565f

1. Removed duplicate code for manifest parsing. 2. Iteration on platform.core instead of manually indexing all possible combinations. 3. Logging improvements. 4. optimizations.

More PR feedback.

0fcaf4a

Renaming checksum to digests. and some formatting cleanups.

doc feedback

a189506

PR feedback

627a24c

PR feedback.

0035f3e

linting errors and unit test fix.

a171812

cleanup.

6629751

local file version works.

178b07c

remove unused stuff.

01615b1

linting and unit tests.

f43585d

nmahendru added 7 commits September 29, 2022 15:42

removing noop formatting.

e273a6d

integration tests.

0668de8

remove code that's adding channel by default.

5226c09

adding hashing for local sources

6cfdd35

integration test for checking the populated shasum.

33e6bdb

added one more integration test.

b6c0a70

Also reduced some duplication for digest calculation.

capturing the appended list.

938d8e6

nmahendru force-pushed the nmahendru/lock_digests_embedded branch from 09e5173 to 938d8e6 Compare September 29, 2022 23:01

use p.SHA256 if present.

dc0dd4a

alecthomas merged commit 2d416f6 into cashapp:master Sep 30, 2022

	LockDigests lockDigestsCmd `cmd:"" help:"Lock the Digests for installed packages using sha256sums." group:"global"`
	LockDigests lockDigestsCmd `cmd:"" help:"Lock the manifest for installed packages." group:"global"`

	This command will build a list of installed packages and their checksums(SHA256) values.
	This command will build a list of installed packages and their digests (SHA256).


		That file can be manually reviewed by the project owner and checked in to the repo.

		On subsequent uses of the hermit environment, the installed dependencies will have a reference checksum

		// If the file is a directory then no checksum is required
		if info.IsDir() {

Lock the digests by adding sha256 values for installed packages. #301

Lock the digests by adding sha256 values for installed packages. #301

Conversation

nmahendru commented Aug 11, 2022 • edited Loading

alecthomas 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

quad 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

quad commented Sep 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmahendru Sep 27, 2022 • edited Loading

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

nmahendru commented Sep 27, 2022

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

quad commented Sep 27, 2022

nmahendru commented Sep 27, 2022

nmahendru commented Sep 27, 2022

quad commented Sep 27, 2022

Choose a reason for hiding this comment

quad 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

alecthomas 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

alecthomas commented Sep 30, 2022

nmahendru commented Aug 11, 2022 •

edited

Loading

quad commented Sep 27, 2022 •

edited

Loading

nmahendru Sep 27, 2022 •

edited

Loading