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

Clean up and expand tests #115

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

dralley
Copy link
Collaborator

@dralley dralley commented Apr 8, 2023

📜 Checklist

  • Commits are cleanly separated and have useful messages
  • A changelog entry or entries has been added to CHANGELOG.md
  • Documentation is thorough
  • Test coverage is excellent and passes
  • Works when tests are run --all-features enabled

@dralley dralley mentioned this pull request Apr 8, 2023
@dralley dralley force-pushed the update-tests branch 5 times, most recently from 95a745a to 2fbfb1a Compare April 9, 2023 04:53
@@ -0,0 +1,77 @@
Name: rpm-feature-coverage
Copy link
Collaborator Author

@dralley dralley Apr 9, 2023

Choose a reason for hiding this comment

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

@DemiMarie I would like to build some nice test RPMs to use (which you are also welcome to use), do you have any recommendations of other things to exercise? e.g.

  • symlinks
  • patches
  • scriptlets
  • triggers

Or overall feedback, or if you know of a good set of existing ones that won't cause licensing issues.

Choose a reason for hiding this comment

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

Some of the things that should be rejected during parsing:

  • INT32_MIN trailer offset
  • Dribbles in the main header
  • Immutable region is not self-consistent
  • Wrong amount of padding (too large or too small)
  • Misaligned header tag data entry
  • Header tag data entry with count 0
  • Header tag data entry count too large
  • Tag appears in both signature and main headers
  • Too many header tag data entries
  • Header too large
  • Tag that should only be in the signature header is found in the main header (this was an exploit in the past)
  • Header-only signature but neither a header+payload signature nor a payload digest (another exploit)
  • Invalid tag (such as the one used by RPM internally for public keys) in the signature or main header
  • Tag data entry of wrong type

Some things that I reject but librpm does not:

  • Dribbles in the signature header.
  • Header tags not sorted
  • Padding not zeroed
  • i18nstring tag has a length more than the i18ntable
  • Weak hash algorithms
  • Invalid UTF-8 in a string tag
  • Suspicious OpenPGP signature
  • Non-ASCII string in the signature header

Things that rpm-oxide should reject:

  • Inconsistent fsverity or IMA signatures (invalid base64, largest one too big, etc)

A simple way to implement this would be to use rpm-oxide for parsing; its API should be able to handle all of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking more about the happy path at the moment. Basically - assuming the file is well constructed, how should I exercise as much surface area as possible in terms of parsing and then querying information about the package.

It will make sense to test invalid packages too but it's not my immediate focus.

Choose a reason for hiding this comment

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

That makes sense. rpm-oxide was initially written to make sure that invalid packages cannot exploit a potentially-vulnerable version of RPM, so rejecting invalid packages was the single highest priority. As a result, it follows the “when in doubt, error out” approach, with large-scale testing on entire repositories used to ensure that it really does accept valid packages. On the other hand, rpm-oxide does not implement package building or installation, as that is simply out of scope for it. From my perspective, it would make sense for rpm-rs to use rpm-oxide for parsing and cryptographic tasks, while implementing a higher-level API itself. rpm-oxide does require librpm for cryptography but I am happy to make that configurable via a trait.

As far as package installation is concerned, I suspect the three trickiest parts are likely to be Lua scriptlets, chroot, and dependency solving, but I have never implemented this code myself so I have no idea. You would be better off asking the rpm-ostree developers, as they have (or in the case of Lua scriptlets, need to) implement this. This would also be a very good opportunity for cooperation.

Copy link
Collaborator Author

@dralley dralley Apr 10, 2023

Choose a reason for hiding this comment

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

Well rpm-rs doesn't (and might not ever) handle package installation, because that would involve working with the system rpmdb, which is a lot more scope given that details may differ between operating systems and across long timeframes. I think it actually makes a lot of sense to just use bindings to librpm in that case.

Anyway, to the original question, we don't need to be concerned with e.g. running scriptlets or doing dependency solving (etc.), but we do want to produce and parse packages with scriptlets or %ghost files or %config or %defattr or patches or different kinds of signatures or all different kinds of dependencies ... (etc.) So along those lines, any ideas of what to add to the spec file and tests?

@dralley dralley force-pushed the update-tests branch 2 times, most recently from 9dbb7b8 to 74a46ea Compare April 9, 2023 20:14
@dralley dralley force-pushed the update-tests branch 3 times, most recently from 6751d16 to ada4c22 Compare April 13, 2023 22:23
src/rpm/headers/header.rs Outdated Show resolved Hide resolved
src/tests.rs Outdated Show resolved Hide resolved
tests/parsing.rs Outdated Show resolved Hide resolved
@dralley
Copy link
Collaborator Author

dralley commented Apr 14, 2023

@drahnr This is ready for a review pass. It could be merged as-is but I think I would like to address a few of the things I called out in comments first. Also open to feedback on the questions I asked

@dralley dralley requested a review from drahnr April 14, 2023 14:50
src/tests.rs Outdated Show resolved Hide resolved
tests/compat.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
tests/common.rs Outdated Show resolved Hide resolved
tests/compat.rs Outdated Show resolved Hide resolved
@drahnr
Copy link
Contributor

drahnr commented Apr 14, 2023

I just rebased your PR in bernhard-foo.

@dralley
Copy link
Collaborator Author

dralley commented Apr 14, 2023

@drahnr Could you just git revert a408803b13 and push that? It should still be in your local git history

@drahnr
Copy link
Contributor

drahnr commented Apr 14, 2023

bernhard-bar

@dralley dralley force-pushed the update-tests branch 12 times, most recently from ec201e4 to 929a51a Compare January 2, 2024 02:15
# symlinks to a file and directory
mkdir -p %{buildroot}/opt/%{name}/symlink_dir/
ln -s ./normal %{buildroot}/opt/%{name}/symlink
ln -s ../dir %{buildroot}/opt/%{name}/symlink_dir/
Copy link
Collaborator Author

@dralley dralley Jan 2, 2024

Choose a reason for hiding this comment

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

To add a bit more specificity to what was going wrong earlier,

without the "mkdir" it yields an error "No such file or directory" if there is a trailing slash, or "Not a directory" if there is not. With the "mkdir" it succeeds but the symlink is placed inside of "symlink_dir" instead of at that path.

I still find this confusing, because when I create that symlink manually without "mkdir" it works fine. Only inside of rpm does it seem to fail. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add that as an information nugget, it will be lost if it's only in the PR

@dralley
Copy link
Collaborator Author

dralley commented Jan 2, 2024

I've cleaned up the commits a bit and made some adjustments to the specfiles. There are a few other simple cleanups as well but nothing significant.

@dralley dralley requested a review from drahnr January 2, 2024 02:24
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@dralley dralley force-pushed the update-tests branch 4 times, most recently from a29dc0e to ffcac7f Compare January 6, 2024 04:44
@dralley dralley marked this pull request as draft January 6, 2024 05:35
@dralley dralley force-pushed the update-tests branch 4 times, most recently from fd98d1b to 34956c8 Compare January 7, 2024 20:41
* Add a roundtrip test
* Move comparison tests to tests/building.rs and extend them
* Use fixture RPMs for new tests in tests/parsing.rs
* Consolidate fixture generation scripts
* Add IMA signing to fixtures
* Create dedicated specfiles for exercising various advanced features
* Add more tests for file attrs and capabilities
* Move test_assets/ to tests/assets/
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.

3 participants