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

Add functionality for verifying tarball signatures to automated ingestion scripts #208

Merged
merged 10 commits into from
Mar 21, 2025

Conversation

bedroge
Copy link
Collaborator

@bedroge bedroge commented Mar 20, 2025

Still testing it, but first tests look good. I'm running a separate instance/fork of the automated ingestion on the Stratum 0, configured to use a test bucket and non-existing target repo. This successfully picked up the tarball+metadata file and their signatures from the test bucket (uploaded by @trz42 in EESSI/software-layer#968):

-rw-r--r--. 1 eessi eessi 124619768 Mar 20 16:12 eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz
-rw-r--r--. 1 eessi eessi       663 Mar 20 16:12 eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz.meta.txt
-rw-r--r--. 1 eessi eessi       878 Mar 20 16:12 eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz.meta.txt.sig
-rw-r--r--. 1 eessi eessi       878 Mar 20 16:12 eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz.sig

The log shows that the signatures were successfully verified:

2025-03-20 16:12:59,176 - DEBUG - Signature for /srv/tmp/tarballs/eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz successfully verified.
2025-03-20 16:12:59,186 - DEBUG - Signature for /srv/tmp/tarballs/eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz.meta.txt successfully verified.

We should do the same for a non-valid signature (and make sure they fail), and for tarballs without a signature file (this should succeed if signatures_required = no).

bedroge added 3 commits March 6, 2025 16:54

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding
@bedroge
Copy link
Collaborator Author

bedroge commented Mar 20, 2025

The staging PR for the test tarball was opened: https://github.com/EESSI/staging/pull/2544. Before merging that PR, I've manually changed the signature of the metadata file to verify that the ingestion would fail, and it did:

2025-03-20 16:28:32,061 - INFO - Tarball 2023.06/software/linux/aarch64/nvidia/grace/1741954310/eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz is ready to be ingested.
2025-03-20 16:28:32,061 - INFO - Verifying its signature...
2025-03-20 16:28:32,276 - DEBUG - Signature for /srv/tmp/tarballs/eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz successfully verified.
2025-03-20 16:28:32,285 - ERROR - Failed to verify signature for /srv/tmp/tarballs/eessi-2023.06-software-linux-aarch64-nvidia-grace-1741954310.tar.gz.meta.txt.
2025-03-20 16:28:32,285 - ERROR - Signature of tarball (or its metadata file) could not be verified!

Right now it only prints this in the log file, and continues. It should also open an issue in the staging repo, I'll have to add that.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding
@bedroge
Copy link
Collaborator Author

bedroge commented Mar 20, 2025

It now opens an issue if the verification fails, see https://github.com/EESSI/staging/issues/2545.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding
@bedroge
Copy link
Collaborator Author

bedroge commented Mar 21, 2025

Another test: I've uploaded a tarball and metadata file without signatures, and set signatures_required = yes. This will print errors to the log and it will ignore the tarball. It doesn't open an issue, as we could possibly get a whole lot of them (but it can be easily added later if we want this).

bedroge added 3 commits March 21, 2025 15:39

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tcharding Tobin C. Harding
@bedroge bedroge marked this pull request as ready for review March 21, 2025 15:19
Copy link
Contributor

@trz42 trz42 left a comment

Choose a reason for hiding this comment

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

Looks good.

  • A few suggestions for naming attributes/variables. (if done would require renaming across the whole script.)
  • A tiny typo.
  • Question about logic when to return False (two places).

Comment on lines +56 to +57
(self.object, self.local_path, self.object_sig, self.local_sig_path),
(self.metadata_file, self.local_metadata_path, self.metadata_sig_file, self.local_metadata_sig_path),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth to rename elements to clearly notify what they refer to and have the naming consistent? For example,

(self.object_remote_path, self.object_local_path, self.object_sig_remote_path, self.object_sig_local_path),
(self.metadata_remote_path, self.metadata_local_path, self.metadata_sig_remote_path, self.metadata_sig_local_path),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This originated from the object terminology used by the boto client, but I agree that it would be more clear to use the names like you proposed. Is it okay to do that in a follow-up PR, as it would require changes across the entire script?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, perfectly fine to do it in a follow-up PR.

(self.metadata_file, self.local_metadata_path, self.metadata_sig_file, self.local_metadata_sig_path),
]
skip = False
for (object, local_file, sig_object, local_sig_file) in files:
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly synchronise naming with potential changes in files array, e.g.,

for (remote_path, local_path, sig_remote_path, sig_local_path) in files:

bedroge and others added 2 commits March 21, 2025 20:07
Fix typo

Verified

This commit was signed with the committer’s verified signature.
apoelstra Andrew Poelstra
Co-authored-by: Thomas Röblitz <[email protected]>
Copy link
Contributor

@trz42 trz42 left a comment

Choose a reason for hiding this comment

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

lgtm

@trz42 trz42 merged commit 735e384 into EESSI:main Mar 21, 2025
20 checks passed
@bedroge bedroge deleted the verify_tarball_signatures branch March 21, 2025 19:45
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.

None yet

2 participants