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

Allow any hashing algorithm in osbuild stage inputs #3514

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

achilleas-k
Copy link
Member

Helper functions that create stage input objects with references always hard-coded sha256: as a prefix/algorithm for the checksum. This prevents the functions from being used in cases where other algorithms are use, like sha1, which is possible with (perhaps older) RPM repositories. The inputs in osbuild a number of hashing algorithms and we should be able to generate stages with other prefixes when necessary.

This PR removes the sha256: prefix in the helper functions and assumes all arguments to these functions provide the correct prefix.

Fixes rhbz#2215043 (RHELPLAN-159871).

This pull request includes:

  • adequate testing for the new functionality or fixed issue

thozza
thozza previously approved these changes Jun 26, 2023
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@achilleas-k
Copy link
Member Author

I also did a test locally with a custom repo using the dummy rpm we use in integration tests and creating a repo with createrepo -s sha512 (it wouldn't allow me to create a sha1 repo on Fedora 38).

Before the fix, grepping through the stage inputs of the manifest in the job store:

# jq .args.manifest.pipelines[1].stages[1].inputs.packages.references[].id de5fe558-bb03-4aca-9713-e0a5acdc7009.json  | grep sha512
"sha256:sha512:795ddd246d8e65bb65e3340c27c1c90b937397f3610e7f308fddb5bfc5e5b29903a82b85ca0f2be1903dc982b10037e4f657a1f474465766f4cf7f03d1572ce2"

and after

# jq .args.manifest.pipelines[1].stages[1].inputs.packages.references[].id c68d6939-6f2e-4cca-89b3-aeb49affce2e.json | grep sha512
"sha512:795ddd246d8e65bb65e3340c27c1c90b937397f3610e7f308fddb5bfc5e5b29903a82b85ca0f2be1903dc982b10037e4f657a1f474465766f4cf7f03d1572ce2"

supakeen
supakeen previously approved these changes Jun 26, 2023
Copy link
Member

@supakeen supakeen 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, perhaps we want to assert the comment in NewFilesInputSourceObjectRef?

@achilleas-k
Copy link
Member Author

Looks good, perhaps we want to assert the comment in NewFilesInputSourceObjectRef?

Yeah good idea as a sanity check.

Helper functions that create stage input objects with references always
hard-coded `sha256:` as a prefix/algorithm for the checksum.  This
prevents the functions from being used in cases where other algorithms
are use, like sha1, which is possible with (perhaps older) RPM
repositories.  The inputs in osbuild a number of hashing algorithms and
we should be able to generate stages with other prefixes when necessary.

Remove the `sha256:` prefix in the helper functions and assume all
arguments to these functions provide the correct prefix.

Update tests to match.
Unit test for the function where the original issue was located.

See rhbz#2215043.
Make sure checksums used in the file input helper functions contain only
1 colon delimiter and it is not at the start of the string.

Adjusted tests to work with new restriction.
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 3ccc2a8 with the main merge-base feaa093). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/4542884599/artifacts/browse

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

👍

@thozza thozza enabled auto-merge (rebase) June 27, 2023 06:30
@thozza thozza merged commit 4f91e95 into osbuild:main Jun 27, 2023
61 checks passed
@achilleas-k achilleas-k deleted the fix/repo-checksums branch June 27, 2023 09:23
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

4 participants