Add checksum verification for all *basic* modules

[marcomontevechi1]

An initial maybe-naive attempt to solve #47

Add sha256sum -c to lnls-get-n-unpack. This covers almost everything but doesn't check areaDetector and motor modules yet.

In this approach, hashes are put manually by the developer, which I initially thought was a good idea...
However, it does feel a little bureaucratic now that I look at it. Is there a better way?
Or maybe should create a script that gets the hashes, but is just an developer auxiliary and is not run at build time?

Tested it and it builds both base and base/musl. Changing the hashes does make it fail like intended.

[henriquesimoes]

Hi, Marco! Thanks for the interest in contributing!

Sorry for the delay on giving you any feedback on this. We're currently going through a accelerator commissioning after a two-month shutdown. In about two weeks, beamlines will be having beam for initial tests. In summary, things are a bit busy by now.

In this approach, hashes are put manually by the developer, which I initially thought was a good idea... However, it does feel a little bureaucratic now that I look at it. Is there a better way?

AFAIK, this is what different Linux distributions do, including ArchLinux, VoidLinux (possibly with a helper script).

Or maybe should create a script that gets the hashes, but is just an developer auxiliary and is not run at build time?

Debian goes that direction, I'd say. Source packages contain SHA1 and SHA256 checksum, but the developer does not need to get into the specifics on how to generate and fill them, since packaging tools will do the job. (It does require you to have the tarball locally to be able to do so, nevertheless).

I think getting this to be done by an automatic script should be easier than documenting the specifics on how to do so.

Tested it and it builds both base and base/musl. Changing the hashes does make it fail like intended.

That's nice!

I haven't looked your code thoroughly, but I did notice three things I think can be addressed beforehand:

• In this project, we use a linear Git history. This means we rebase patches rather than creating merge commits. Thus, drop 351574e and rebase your branch on top of your current main. I suggest taking a look at git-rebase(1) manpage to familiarize yourself with the command, if you haven't used it before.
• You should follow our commit message header (and body) pattern (that is not properly documented yet), but that can be easily deduced by git logging files you have touched. In this case, it is mostly a base image change, so all of them would have a base: prefix followed by a title starting with an imperative verb in lowercase and ending with a dot.
• Your patches should not introduce bugs fixes to your own proposed patches. They should be well separated, atomic, and should document all relevant tradeoffs and rationale you made to implement the way you did. If you prefer, when we review them in details, we can tell which ones to squash, reorder or rewrite the message if any.

These things might seem not important at first, but in the long run they make a huge difference, since the Git history will be full of meaningful rationale that can easily queried by git-blame(1) and related Git tooling, and ease decision making. This is close to what both Linux kernel and Git project follow themselves, and is a good practice to collaborative projects.

For the changes themselves, I'll see if I can go through them soon.

[marcomontevechi1]

Greetings!

Thus, drop 351574e and rebase your branch on top of your current main.

Done!

You should follow our commit message header (and body) pattern

I did that in the titles for now. Will do better in the messages after I fix the next step:

Your patches should not introduce bugs fixes to your own proposed patches. They should be well separated, atomic, and should document all relevant tradeoffs and rationale you made to implement the way you did. If you prefer, when we review them in details, we can tell which ones to squash, reorder or rewrite the message if any.

For now I simply squashed the most obvious patch which was a fix to an obvious broken command. However, the commits are definitely not atomic since I had initially decided to add a new lnls-get-n-unpack from scratch and replace the old one with the new after it was all ready. I had decided to do that because although it's a simple bash script, at the time I didn't feel I could turn the old script into the new one in an small-increment series of commits.

What I intend to do now is to reorganize the commits in such a way that they still create a new lnls-get-n-unpack from scratch, but replace it with the new one only after it is actually finished (what I should have done from the start, anyway).

Please tell me if that is acceptable or if you require the original script to be modified in small increments for the PR to be accepted.

Cheers!

[gustavosr8]

Hey Marco!

Please tell me if that is acceptable or if you require the original script to be modified in small increments for the PR to be accepted.

Talking with @henriquesimoes and @ericonr we agreed that it's better if the changes are made in the original script to preserve the commit history from previous patches.

[marcomontevechi1]

Hi, @gustavosr8!

I made the suggested changes, or at least as long as I can tell. Please feel free to review the current code.
Unfortunately the last commit is kinda big, but it follows the logic of being the smallest incremental/logical step that can be done after adding the shacheck function without breaking the build. All changes added beyond those in lnls-get-n-unpack.sh are just passing the needed hashes as arguments to the correct functions/files anyway.

I tested both builds here and they seem to work.

Please let me know if there is anything else that should be done.

[gustavosr8]

Hey Marco!

I think that a rebase is missing. We added support for modbus in the last release, and the CI fails when building this module.

Unfortunately the last commit is kinda big, but it follows the logic of being the smallest incremental/logical step that can be done after adding the shacheck function without breaking the build.

About that, WDYT on making a previous commit just adding the module's SHA256 and mentioning that it will be used in the future, separating it from the commit where you actually implement the SHA check and use it in the download and install functions?

[marcomontevechi1]

I apparently did a wrong force push after the rebase to add modbus and erased everything 🫠
The arcane art of recovering lost commits like these is still unbeknownst to this poor mortal.
Will start rewriting the stuff.

About that, WDYT on making a previous commit just adding the module's SHA256 and mentioning that it will be used in the future, separating it from the commit where you actually implement the SHA check and use it in the download and install functions?

Yup, seem fine to me. I'm a bit bothered by the thought that it won't be a "completely atomic" commit, but it won't break the build anyway.

[marcomontevechi1]

Managed to recover the commits after rebasing to add latest changes.
Split the last commit into two, first adding the expected hashes and then adding the actual function checking. Tested the build here and it seems to work.

[gustavosr8]

Hey poor mortal!
Now it looks good to me (and also for the ci) :)
Lets just wait for a double check from @henriquesimoes or @ericonr before merging.
Thanks for your patience so far!

[ericonr]

With #88 merged, we can avoid repeating all those variable names! Just move the checksum variables into the relevant *_versions.sh file :D

[henriquesimoes]

Hi, Marco!

I hope this message finds you well.

After being late almost a year now, I have finally taken the opportunity to review your changes carefully. I've made several comments. I didn't review the hashes themselves (nor the way they are introduced), because several of them will be different now, and they should be placed in the version files given the latest directory structure.

Please, let us know if you are still interested on working on this. If not, we can rework the patches ourselves and keep the appropriate authorship information.

Thanks for first working on this.

[henriquesimoes]

Nit: about this change's commit message: I often see a trailer for specifying who suggested the change (when that's relevant). The message would look like this:

base: remove unneeded ';' from lnls-get-n-unpack.

Suggested-by: Gustavo de Souza dos Reis <[email protected]>

If there was a really important discussion in the provided link, it should also be summarized in the message itself. Otherwise, the project's Git history would depend on the availability of the external platform (in this case, GitHub) to be understandable.

In general, I prefer to place links in their own line at the end of the message (see 76c2b86 for an example), pretty much like bibliographies. That makes it easier to read the message itself.

[marcomontevechi1]

Done in commit 8bce3ff.

[henriquesimoes]

Just nitpicking a bit more. They are usually used as trailers, which means they must come last. I don't know a proper way to provide a URL to the reference your usage of a trailer. I think a better way is to include a very brief mention to the message body itself, which is linked instead. Something like this:

base: remove unneeded ';' from lnls-get-n-unpack.

Command separator is not needed if there are no commands afterwards.
Drop it as suggested during review [1].

[1] https://github.com/cnpem/epics-in-docker/pull/80#discussion_r1840862700

Suggested-by: Gustavo de Souza dos Reis <[email protected]>

Note that I've also rewritten the body as a single sentence.

I'm just nitpicking on this because I think it will be valuable for your future contributions. That's certainly an overkill for this typo change; although it is a quite didactic example.

[marcomontevechi1]

Hi! I did lose traction with this, but it would sure be nice to finish. The reminder is very much welcome :)

I will work on this no later than until the end of this weekend.
Of course, if for any reason this is a priority and you need it before then, feel free to rework the patches internally.

[henriquesimoes]

Hi! I did lose traction with this, but it would sure be nice to finish. The reminder is very much welcome :)

Good to know you're still somewhat interested. ;)

I will work on this no later than until the end of this weekend.
Of course, if for any reason this is a priority and you need it before then, feel free to rework the patches internally.

No hurry. I just found some time to review it after a long period of time. It's not a priority for us at all right now. By the way, that's why it took us so long to give you further feedback.

Take your time to iterate over the review comments carefully. If we ever consider this a priority in our end, we'll let you know.

We do want your contributions to be integrated nevertheless. We are considering it part of the roadmap for the v1.0.0 release, which doesn't have a target date.

[marcomontevechi1]

So, about the extra blank lines as I said in previous comments the rebase+forcepush confused me.

I removed every extra line immediately after function init and before funtion end, but kept blank lines between blocks that seem to execute different tasks. If you prefer to have every blank line removed or any other criteria, please feel free to open more discussions and I will make sure to not get lost again with the diffs.

[marcomontevechi1]

My pipeline seems to be failing although locally everything works...

I'm wonderin if this could have something to do with access to ghcr?

[gustavosr8]

I'm wonderin if this could have something to do with access to ghcr?

Hi @marcomontevechi1! Yeah, I had to allow it to run, but seems to be passing without any issues now

[gustavosr8]

Small nit on e289f22
s/identated/indented

For reference: https://www.merriam-webster.com/dictionary/indent

[marcomontevechi1]

Small nit on https://github.com/cnpem/epics-in-docker/commit/e289f22f993512b6500e5918fae6e537d0565f60
s/identated/indented

For reference: https://www.merriam-webster.com/dictionary/indent

Okay, fixed in commit message 5301220

[henriquesimoes]

I think this is getting into good shape. I'm leaving some minor suggestions in the threads, mostly nitpicks.

I'm still planning on properly reviewing the hashes variables. I just skimmed through them as of now. But they should be good, as the image builds fine.

Besides, here goes some commit message comments:

commit 5301220
Author: marcofilho [email protected]
Date: Wed Nov 20 22:50:59 2024 +0100

base: encapsulate lnls-get-n-unpack in functions.

This script will grow a bit, so I organize it before making it grow.

"so I organize it [...]" -> "so organize it [...]". As with the title, the
commit message can give orders to the codebase. This way messages becomes less
verbose, as we don't need to say things like "this commit", "these changes",
etc. It also makes clear what is the intended effect of applying the commit.
That's an approach the Git developers use as a convention internally, for instance.

Function content is indented. Functions parse_arguments and download
are used at the end of script.

Help function is added to be called when script is misused. Format of
arguments passed to script will change in future commits, so this might
help clarify when wrong usage is made.

Similar to common ArgParse usage in python, parse_arguments decides

Suggestion: "common" -> "usual", as it is not intended to mean "shared usage",
but actually "frequent usage".

Nit: "ArgParse" -> "argparse" (as it is the module name) or "ArgumentParser" as
the class name. I'm not sure which one you intended to refer to.

Nit: "python" -> "Python"

which arguments will be passed to download.

---

commit f8356a1
Author: marcofilho [email protected]
Date: Sat Oct 18 16:50:15 2025 +0200

base: add sha256 expected hashes structure.

Suggestion: "base: add SHA-256 hashes for existing modules."

Manually downloaded and checked each sha256sum for each epics module
that is downloaded with lnls-get-n-unpack. Added the expected sums to
each *_version.sh file.

"Each EPICS module that is downloaded with lnls-get-n-unpack was manually
downloaded and had its sha256sum calculated. The expected sums are added to the
corresponding *_version.sh files."

Starting with "Manually downloaded and checked" seems to be missing a subject.
Reworking as a passive voice makes the problem disappear without having to
indicate one (which is unusual in these messages, I'd say).

The variables are not actually used in this commit. Next commit shall
add the actual sha256sum checking.

[henriquesimoes]

Just nitpicking a bit more. They are usually used as trailers, which means they must come last. I don't know a proper way to provide a URL to the reference your usage of a trailer. I think a better way is to include a very brief mention to the message body itself, which is linked instead. Something like this:

base: remove unneeded ';' from lnls-get-n-unpack.

Command separator is not needed if there are no commands afterwards.
Drop it as suggested during review [1].

[1] https://github.com/cnpem/epics-in-docker/pull/80#discussion_r1840862700

Suggested-by: Gustavo de Souza dos Reis <[email protected]>

Note that I've also rewritten the body as a single sentence.

I'm just nitpicking on this because I think it will be valuable for your future contributions. That's certainly an overkill for this typo change; although it is a quite didactic example.

[henriquesimoes]

What do you think about having *_SHA256 variables interleaved with the *_VERSION variables? This way, all variables related to a module would be close together. This should be easier to edit them both when upgrading to a new module release.

As this is a tiresome thing to do (and undo), please wait for @gustavosr8's and @ericonr's opinions before applying this suggestion.

[gustavosr8]

Hmm, having the same module's variables together seems more direct to tell if something is missing.

[marcomontevechi1]

Downloading the packages and checking the hashes is more work than organizing them in the files... at least that's how it feels for me.

I added them as interleaved variables as suggested but it would be no trouble to put it back on the way it was before if you guys come to the conclusion that it's better that way.
I initially liked the ideas of having them in separate blocks by function (sha or version variables), but then there is the effort of checking if they are in the same order...

[henriquesimoes]

Thanks a lot for this tiresome work of (re)calculating all hashes for existing modules. This is pretty much appreciated.

[marcomontevechi1]

No problemo :)
I kinda chose this task because it was less complex and more of a manual thing anyway.
I thought for a minute about adding a helper script as discussed in previous comments, but I'm not so sure now...

A helper script that automatically downloads and checks everything seems to kinda break the whole purpose of checking the SHAs. On the other hand, an atomic script that only downloads one module using the specified version seems unnecessary code.

[henriquesimoes]

I kinda chose this task because it was less complex and more of a manual thing anyway.

Okay. Good to know. :)

A helper script that automatically downloads and checks everything seems to kinda break the whole purpose of checking the SHAs.

It depends. One way of approaching this is to inspect the tarball content before calculating its hash. But that's true when manually generating the hashes as well. Another approach is to simply trust the first seen tarball, and only ensure that is the same from then on. We still need to discuss which approach works the best for us, but both of them already solve one problem: attacks that attempt to change a tarball already in use without us noticing it.

On the other hand, an atomic script that only downloads one module using the specified version seems unnecessary code.

Agreed.

[henriquesimoes]

I have just two more minor comments in the threads.

In general, the only thing is missing is documentation now. I think the procedure on how to deal with checksum verification is valuable thing to be documented. In Érico's new PR, we open room in a document for writing such things.

We still need to discuss which approach is the best: recommending some kind of manual inspection of the tarballs upon upgrade/addition or using the Trust on First Use (TOFU) approach.

[henriquesimoes]

Nit: this comment should be above the shift or be dropped. ;)

[henriquesimoes]

Nit: Something I hadn't thought before: placing it right after _VERSION variables is cleaner, isn't it?

It should just affect install_from_github, since download_from_github and others already take it that way.

[henriquesimoes]

I kinda chose this task because it was less complex and more of a manual thing anyway.

Okay. Good to know. :)

A helper script that automatically downloads and checks everything seems to kinda break the whole purpose of checking the SHAs.

It depends. One way of approaching this is to inspect the tarball content before calculating its hash. But that's true when manually generating the hashes as well. Another approach is to simply trust the first seen tarball, and only ensure that is the same from then on. We still need to discuss which approach works the best for us, but both of them already solve one problem: attacks that attempt to change a tarball already in use without us noticing it.

On the other hand, an atomic script that only downloads one module using the specified version seems unnecessary code.

Agreed.