GNOME Bugzilla – Bug 797177
Require tarball checksum verification for all recipes
Last modified: 2018-10-30 17:58:02 UTC
The checksum is stored in the recipe and must be updated when a recipe is updated to a new version. This means we no longer need to rely on upstream download URLs for source tarballs to be trustworthy for eternity.
Created attachment 373705 [details] [review] cerbero: Make FatalError exceptions more readable When the exception originates in a recipe, print the recipe path and the line number with the error message. Otherwise, only print the error message because that information is useless to the average cerbero user. In any case, never print a traceback on FatalError
Created attachment 373706 [details] [review] cerbero: Use and require checksums for all tarball recipes The sha256 checksum must be specified with the `tarball_checksum` property on the recipe. If that property is not defined, a helpful error message is printed.
Created attachment 373707 [details] [review] recipes: Add tarball checksums for all recipes This should cover all recipes; even those that aren't built by default
Review of attachment 373707 [details] [review]: Could have been just name "checksum" instead of "tarball_checksum" ... haha, kidding, I don't really care. Note, I didn't recalculate all the checksum :-D
Review of attachment 373705 [details] [review]: Yeah, finally, thanks.
Review of attachment 373706 [details] [review]: ::: cerbero/build/source.py @@ +128,3 @@ cached_file = os.path.join(self.config.cached_sources, self.package_name, self.tarball_name) + if not redownload and os.path.isfile(cached_file) and self.verify(cached_file, fatal=False): I'm wondering if checking the cache won't be quite an overhead for the safety. What we could do instead is download to <filename>.unchecked, and if it succeed validate, move it back to it's real name. And then just trust the cache. @@ +174,3 @@ + if checksum != self.tarball_checksum: + movedto = fname + '.failed-checksum' + os.replace(fname, movedto) I'm not familiar with replace, does it override if there is already a .failed-checksum file ? Just asking if we need to remove ancient failed download first.
(In reply to Nicolas Dufresne (ndufresne) from comment #4) > Review of attachment 373707 [details] [review] [review]: > > Could have been just name "checksum" instead of "tarball_checksum" ... haha, > kidding, I don't really care. Note, I didn't recalculate all the checksum :-D Just to entertain the bikeshed for a moment, I thought `tarball_checksum` would be good alongside `tarball_dirname` and `tarball_name`. :) (In reply to Nicolas Dufresne (ndufresne) from comment #6) > Review of attachment 373706 [details] [review] [review]: > > ::: cerbero/build/source.py > @@ +128,3 @@ > cached_file = os.path.join(self.config.cached_sources, > self.package_name, self.tarball_name) > + if not redownload and os.path.isfile(cached_file) and > self.verify(cached_file, fatal=False): > > I'm wondering if checking the cache won't be quite an overhead for the > safety. What we could do instead is download to <filename>.unchecked, and if > it succeed validate, move it back to it's real name. And then just trust the > cache. > It's actually not a lot of overhead at all. It takes <2 seconds to verify all the tarball checksums in gstreamer-1.0, which is a trivial amount of time compared to building stuff. I tested with: time ./cerbero-uninstalled fetch-package gstreamer-1.0 So I think it's good to keep the implementation simple. It's also a good way to know and recover easily if a file in your cache got corrupted somehow. > @@ +174,3 @@ > + if checksum != self.tarball_checksum: > + movedto = fname + '.failed-checksum' > + os.replace(fname, movedto) > > I'm not familiar with replace, does it override if there is already a > .failed-checksum file ? Just asking if we need to remove ancient failed > download first. Yes, os.replace is a cross-platform API to overwrite the existing file: https://docs.python.org/3/library/os.html#os.replace
Review of attachment 373706 [details] [review]: Ok, sounds good.
Thanks for the review! Attachment 373705 [details] pushed as 2f2ec83 - cerbero: Make FatalError exceptions more readable Attachment 373706 [details] pushed as bdc074c - cerbero: Use and require checksums for all tarball recipes Attachment 373707 [details] pushed as 7949284 - recipes: Add tarball checksums for all recipes
*** Bug 773491 has been marked as a duplicate of this bug. ***