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.

Sign up for GitHub

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

Jump to bottom

buildchain: embed ISO MD5 in the generated image #3032

Merged
merged 1 commit into from
Jan 14, 2021
Merged

buildchain: embed ISO MD5 in the generated image #3032

merged 1 commit into from
Jan 14, 2021

Conversation

NicolasT
Copy link
Contributor

The isomd5sum tools allow to embed a hash of data sectors found in an
ISO file in an otherwise unused section, hence allowing the integrity of
(the data sectors of) an ISO image to be checked. This is, e.g., also
done for RHEL/CentOS/Fedora ISOs (their integrity can be checked at boot
time).

This commit adds a call to implantisomd5 at the end of the buildchain,
right after the ISO is created and before its SHA256 is calculated.

Given this, one can run checkisomd5 on a resulting ISO file after
download to ensure it's not corrupted.

Fixes: #3026
See: #3026
See: https://github.com/rhinstaller/isomd5sum

@NicolasT NicolasT added kind:enhancement New feature or request topic:build Anything related to building steps complexity:easy Something that requires less than a day to fix python Pull requests that update Python code labels Jan 13, 2021
@NicolasT NicolasT requested a review from a team as a code owner January 13, 2021 14:37
@bert-e
Copy link
Contributor

bert-e commented Jan 13, 2021

Hello nicolast,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Jan 13, 2021

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

@NicolasT NicolasT force-pushed the improvement/isomd5 branch 2 times, most recently from 6d2dfe8 to bf2c8f1 Compare January 13, 2021 15:05
gdemonet
gdemonet reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@gdemonet gdemonet left a comment

Choose a reason for hiding this comment

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

Maybe worth adding a mention of this isomd5sum package in the developer docs (under docs/developer/building/requirements.rst)?

@NicolasT
Copy link
Contributor Author

As suggested (@gdemonet) I added the new requirement to the developer documentation. Also extended the Eve step which currently only tests integrity using the SHA256SUM file to also use checkisomd5 if there's some sign the ISO contains the relevant metadata (checkisomd5 errors out with the same exit code both when there's a checksum mismatch, as well as when no checksum is embedded, so no way to dispatch). The detection mechanism is... 'crude', but shouldn't result in false positives. It could be removed later once all ISOs we use in CI (ideally starting with 2.9?) contain the checksum.

gdemonet
gdemonet previously approved these changes Jan 14, 2021
View reviewed changes
@NicolasT
buildchain: embed ISO MD5 in the generated image
5a92338 
The `isomd5sum` tools allow to embed a hash of data sectors found in an
ISO file in an otherwise unused section, hence allowing the integrity of
(the data sectors of) an ISO image to be checked. This is, e.g., also
done for RHEL/CentOS/Fedora ISOs (their integrity can be checked at boot
time).

This commit adds a call to `implantisomd5` at the end of the buildchain,
right after the ISO is created and before its SHA256 is calculated.

Given this, one can run `checkisomd5` on a resulting ISO file after
download to ensure it's not corrupted.

Also mention the check in the docs, the `implantisomd5` requirement in
the developer docs, and add some code in the CI scripts to validate the
ISO using the embedded MD5 if present, next to the `SHA256SUM`.

Fixes: #3026
See: #3026
See: https://github.com/rhinstaller/isomd5sum
@NicolasT
Copy link
Contributor Author

/approve

@bert-e
Copy link
Contributor

bert-e commented Jan 14, 2021

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/2.8

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5
  • development/2.6
  • development/2.7

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Jan 14, 2021

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/2.8

The following branches have NOT changed:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5
  • development/2.6
  • development/2.7

Please check the status of the associated issue None.

Goodbye nicolast.

@bert-e bert-e merged commit a0b0c6f into development/2.8 Jan 14, 2021
@bert-e bert-e deleted the improvement/isomd5 branch January 14, 2021 20:44
@NicolasT NicolasT mentioned this pull request Jan 15, 2021
Closed
gdemonet added a commit that referenced this pull request Apr 12, 2021
@gdemonet
ci: Always check embedded MD5 in ISO
6521108 
Since #3032 (merged in 2.8), all our ISOs include an embedded MD5
checksum. We had an empirical check to know whether this checksum was
indeed present, since we also need to verify ISOs from (N-1) when
running upgrade/downgrade tests from version (N).

Now, in 2.9, all ISOs we expect should include this checksum. The
empirical check is no longer needed, and we remove it.
We also add a common `requirements.sh` to install the required package
for openstack workers which were only used to spawn other VMs through
Terraform, since these workers are responsible for validating the
integrity of retrieved artifacts.
@gdemonet gdemonet mentioned this pull request Apr 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdemonet gdemonet gdemonet approved these changes

@slaperche-scality slaperche-scality slaperche-scality left review comments

Assignees
No one assigned
Labels
complexity:easy Something that requires less than a day to fix kind:enhancement New feature or request python Pull requests that update Python code topic:build Anything related to building steps
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NicolasT @bert-e @slaperche-scality @gdemonet