Clean up and expand tests #115

dralley · 2023-04-08T04:04:06Z

📜 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 · 2023-04-09T05:02:16Z

test_assets/fixture_packages/SPECS/rpm-feature-coverage.spec

@@ -0,0 +1,77 @@
+Name:           rpm-feature-coverage


@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.

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.

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.

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.

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?

test_assets/fixture_packages/build_packages.sh

src/rpm/headers/header.rs

src/tests.rs

tests/parsing.rs

dralley · 2023-04-14T14:49:51Z

@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

src/tests.rs

tests/compat.rs

.github/workflows/ci.yml

tests/common.rs

tests/compat.rs

drahnr · 2023-04-14T15:18:47Z

I just rebased your PR in bernhard-foo.

dralley · 2023-04-14T15:26:51Z

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

drahnr · 2023-04-14T15:35:16Z

bernhard-bar

dralley · 2024-01-02T02:20:01Z

tests/assets/SPECS/rpm-file-attrs.spec

+# 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/


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. 🤷

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

dralley · 2024-01-02T02:22:38Z

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.

.github/workflows/ci.yml

* 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/

closes rpm-rs#207

drahnr · 2024-11-18T10:28:22Z

src/rpm/headers/header.rs

+    pub digest: String,
+    pub algo: DigestAlgorithm,


Please provide accessor methods and mark these as pub(crate)

drahnr · 2024-11-18T10:29:04Z

src/rpm/headers/header.rs

@@ -575,7 +575,7 @@ impl IndexHeader {
 }

 /// A single entry within the [`IndexHeader`](self::IndexHeader)
-#[derive(PartialEq)]
+#[derive(Clone, PartialEq, Eq)]


I don't think we should add these unless necessary, particularly because the type is internal. For external types I think they're valid addenda

drahnr · 2024-11-18T10:29:35Z

src/rpm/headers/types.rs

+//     REG		=  8,	/*!< regular file */
+//     LINK	= 10,	/*!< hard link */
+//     SOCK	= 12	/*!< socket */
+// } rpmFileTypes;


Can we represent this as another bitfield?

dralley · 2024-11-18T14:12:19Z

Oh, I just rebased this PR, it's not ready for merging yet or anything.

dralley mentioned this pull request Apr 8, 2023

always digest #111

Merged

dralley force-pushed the update-tests branch 5 times, most recently from 95a745a to 2fbfb1a Compare April 9, 2023 04:53

dralley commented Apr 9, 2023

View reviewed changes

dralley force-pushed the update-tests branch 2 times, most recently from 9dbb7b8 to 74a46ea Compare April 9, 2023 20:14

dralley commented Apr 9, 2023

View reviewed changes

test_assets/fixture_packages/build_packages.sh Outdated Show resolved Hide resolved

dralley force-pushed the update-tests branch 3 times, most recently from 6751d16 to ada4c22 Compare April 13, 2023 22:23