Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding functionality for dirhash in library #223

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

matglas
Copy link
Contributor

@matglas matglas commented Apr 29, 2024

Related to: in-toto/witness#436

This a different approach to #65. It captures a dirhash for material and product if the dir is mentioned as an argument.

witness run --dirhash-glob node_modules/* --step build -- npm clean-install

The argument can be repeated multiple time and globs with ** are not allowed.

The output will generate a dirhash based on the algorithm use by golang in https://pkg.go.dev/golang.org/x/mod/sumdb/dirhash and is the same as described in the in-toto digest spec (here)

The subject for a dirhash will be https://witness.dev/attestations/product/v0.1/dir:node_modules/example/.

The policy works correctly and also artifactsFrom are correctly checked.

Todo

  • allow verify to check not only file subject but also dir subject
  • decide on the copying of the glob library into the go-witness library.
  • discuss what additional docs on the library would be benificial

attestation/context.go Show resolved Hide resolved
attestation/file/file.go Show resolved Hide resolved
attestation/product/product.go Outdated Show resolved Hide resolved
attestation/product/product.go Show resolved Hide resolved
cryptoutil/dirhash.go Show resolved Hide resolved
@ChaosInTheCRD
Copy link
Collaborator

Hey! thanks for submitting this 😄. It looks like this is the sort of functionality we were looking for after the discussion on the community call, so thanks a lot for following up with it.

After looking through things, it seems as though everything looks good, although the use of "github.com/gobwas/glob" worries me a tad since it's a package that belongs to an inactive user and hasn't been committed to for 6 years.

It's probably a tad ironic since at this time we don't have any strong policy over the kind of packages we are and are not happy to pull in. But maybe in this case, a bit of copying might be better than depending? 🤔 Just a thought.

I would add also that we need to get tests written before this is merged, and docs updated in github.com/in-toto/witness before in-toto/witness#436 is merged.

@matglas
Copy link
Contributor Author

matglas commented Apr 30, 2024

Thanks for the input. What is ok OSS wise in copying code?

@adityasaky
Copy link
Member

I haven't had a chance to look at this PR, but I wanted to check if it's consistent with the implementation in in-toto/in-toto. Is that the case? Thanks!

@matglas
Copy link
Contributor Author

matglas commented Apr 30, 2024

I haven't had a chance to look at this PR, but I wanted to check if it's consistent with the implementation in in-toto/in-toto. Is that the case? Thanks!

Yes as far as the output of the commandline example in the code there goes and what I saw in the digest documentation. I have not checked the code in all its detail.

@matglas
Copy link
Contributor Author

matglas commented May 1, 2024

@ChaosInTheCRD I updated and fixed the tests to do some dirhash testing.

@matglas matglas changed the title chore: Adding functionality for dirhash. Adding functionality for dirhash in library May 1, 2024
@matglas matglas force-pushed the dirhash-glob branch 2 times, most recently from ae2634a to dad9da0 Compare May 14, 2024 07:31
@matglas
Copy link
Contributor Author

matglas commented May 14, 2024

Cherry-picked all commits onto latest main to fix DCO issues.

@matglas matglas force-pushed the dirhash-glob branch 2 times, most recently from 3723727 to a8841d0 Compare May 20, 2024 12:08
@jkjell jkjell force-pushed the dirhash-glob branch 5 times, most recently from af27659 to 0b19316 Compare August 12, 2024 04:38
@matglas matglas force-pushed the dirhash-glob branch 3 times, most recently from 3ea503c to e675966 Compare August 22, 2024 13:10
@matglas
Copy link
Contributor Author

matglas commented Aug 23, 2024

@jkjell all should be good to merge now. There are not breaking changes to witness. The usage will be made available in the other repo. But even with the changes in it should not break any other behavior.

Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @matglas! 🎉

@jkjell jkjell merged commit 5d41c36 into in-toto:main Aug 24, 2024
12 checks passed
@matglas
Copy link
Contributor Author

matglas commented Aug 25, 2024

Great! Thank you!

@matglas matglas deleted the dirhash-glob branch August 25, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants