always digest #111

drahnr · 2023-04-01T19:33:18Z

@dralley includes the part of your PR, some additional cleanups, and the direction of using a fn verify_digest explicitly, so the user can opt-in explicitly to have either or both checked. The fn verify_digest is not feature gated, while the fn verify_signature is.

I hope this clarifies the conversation had in #106

Potentially I'll complete this PR soon™

dralley · 2023-04-07T20:53:19Z

FYI, I'm working on an overhaul of the tests right now now, so it might be best to drop that commit to avoid conflicts - but I will try to introduce a similar split by the time I'm done with it.

Work in progress here: #115

drahnr · 2023-04-08T06:29:50Z

@drally please hold off, I'll complete these changes early next week

dralley · 2023-04-08T06:55:16Z

Well, I was hoping to finish mine tomorrow or by the end of the weekend...

Note that I'm talking exclusively about the tests and not the digest PR. It's separate.

dralley · 2023-04-09T20:40:47Z

I don't, in general, like keeping tests that could (and ought to be) integration tests inside src/. The tests commit goes further in that direction and so I'd prefer we didn't do that. Just letting you know before you put too much work into it.

Copying 32 bytes is cheaper than creating a ptr/reference.

cmeister2 · 2023-04-14T12:46:13Z

src/rpm/headers/header.rs

@@ -136,17 +136,17 @@ where
        Ok(())
    }

-    pub(crate) fn find_entry_or_err(&self, tag: &T) -> Result<&IndexEntry<T>, RPMError> {
+    pub(crate) fn find_entry_or_err(&self, tag: T) -> Result<&IndexEntry<T>, RPMError> {


I know this isn't your code, but find_entry_or_err seems a little redundant on a function that returns a Result. I don't see this pattern on any of the standard library (https://doc.rust-lang.org/std/index.html?search=or_err). Might be worth fixing up as part of this?

With regards to this specific change - I think this makes the interface a little nicer if you're only comparing to "instances" of Tags (as per the tests). I'm assuming that Tags are not very big and the general use for this interface is to search for specific Tags.

The tags are u32s in disguise, hence there is no point in taking a reference.

I'll leave the API as is, since my expectation on a find would be to return an Option, but this does return a Result.

cmeister2

Oops, wrong account. Looks good :)

(I can't really comment on the design aspects here - they look reasonable though! The Rust I've already commented on.)

dralley · 2023-04-14T14:30:16Z

@drahnr I had asked for "split up tests" to be dropped : /

drahnr · 2023-04-14T14:35:57Z

I am sorry, I forgot about that. I would generally prefer if we could sync on any upcoming work, rather than fighting on changes to merged or skipped, but rather avoid competing changes in the first place if they become to cumbersome to do a git-rebase.

dralley · 2023-04-14T15:05:20Z

@drahnr Since the commits got squashed, the commit a408803b13 isn't present in the git tree, which makes it kind of a pain to revert. Since you still have it in your local git history, can you revert it and push?

dralley · 2023-04-15T04:06:25Z

src/rpm/package.rs

@@ -142,12 +191,10 @@ impl RPMPackage {
    ///
    ///
    #[cfg(feature = "signature-meta")]
-    pub fn verify_signature<V>(&self, verifier: V) -> Result<(), RPMError>
+    pub fn verify_signature<V>(&self, verifier: V) -> Result<SignatureVerificationOutcome, RPMError>


@drahnr I'm confused by this. Nothing actually returns SignatureVerificationOutcome::Fail so it doesn't seem useful? Even if it did, encoding an error condition in the success path feels very wrong.

That's correct, fix eta today

dralley · 2023-04-15T04:06:48Z

src/rpm/package.rs

+            Ok(DigestVerificationOutcome::Match)
+        } else {
+            Ok(DigestVerificationOutcome::Mismatch)
+        }


A test for this would be good.

Like above though I'm confused about the error strategy. If I were to write some fairly intuitive code such as

pkg.verify_digest()?;

it effectively won't actually be checking the digests because we've created a separate feedback mechanisms, and we're not marking the return value #[must_use] to force the user to check it. It feels like a footgun.

Also with this approach there is no way to tell which digest failed or get any additional context back? We already have a ValidationError variant for signature mismatch, I'm just confused why we're doing something different for digests.

Rationale was, that the RPM format is fine, the signature just failed to verify. And the verfication might simply caused by a missing key.

But I think for the common usage pattern you're correct that it poses a footgun.

VerificationError could have subtypes potentially

Actually there is KeyNotFound error already https://github.com/rpm-rs/rpm/pull/122/files#diff-6ff19a1ff611f3835b5a025663cf4dec901ed914626f78847414836439bd1a79L77

dralley · 2023-04-15T04:56:08Z

src/rpm/builder.rs

+        } = RPMPackage::create_digests_from_readers(
+            &mut header.as_slice(),
+            header_and_content_cursor,
+        )?;


Modern versions of RPM don't actually generate these digests, but they would still be needed for e.g. EL7 packages. Maybe in a year when it goes EOL we can ditch this code.

This function can only be used for the legacy digests though, because the modern ones (PAYLOADDIGEST, PAYLOADDIGESTALT which I am currently adding support for) go into the main header, which has already been generated at this point.

dralley marked this pull request as draft April 1, 2023 19:36

drahnr added 12 commits April 14, 2023 08:29

Always build digests of header and content

17405dc

more

627109d

chore: fmt toml

a464d37

fx

0aa6a10

fix

d589fb8

dead comment

80d1a53

fixes

42c89f1

split up tests

a408803

finish digest verification

0cf58bc

remove reference usage for IndexTag types

1330cd3

Copying 32 bytes is cheaper than creating a ptr/reference.

fixins

9646853

fallout

eaf0eb6

drahnr force-pushed the bernhard-always-digest branch from 41159e2 to eaf0eb6 Compare April 14, 2023 06:36

drahnr marked this pull request as ready for review April 14, 2023 06:36

feature gate fixes

397fa5d

drahnr requested review from cmeister2 and dralley and removed request for cmeister2 April 14, 2023 11:42

cmeister2 reviewed Apr 14, 2023

View reviewed changes

drahnr requested a review from cmeister2 April 14, 2023 13:14

This comment was marked as duplicate.

Sign in to view

cmeister2 approved these changes Apr 14, 2023

View reviewed changes

drahnr merged commit 815ddb3 into master Apr 14, 2023

drahnr deleted the bernhard-always-digest branch April 14, 2023 14:12

drahnr mentioned this pull request Apr 14, 2023

Always create package digests #106

Closed

5 tasks

dralley reviewed Apr 15, 2023

View reviewed changes

drahnr mentioned this pull request Apr 18, 2023

revert: remove inconsistent use of outcome enums, add another error v… #122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always digest #111

always digest #111

drahnr commented Apr 1, 2023 •

edited

Loading

dralley commented Apr 7, 2023 •

edited

Loading

drahnr commented Apr 8, 2023

dralley commented Apr 8, 2023

dralley commented Apr 9, 2023

cmeister2 Apr 14, 2023

cmeister2 Apr 14, 2023

drahnr Apr 14, 2023

drahnr Apr 14, 2023

This comment was marked as duplicate.

cmeister2 left a comment •

edited

Loading

dralley commented Apr 14, 2023 •

edited

Loading

drahnr commented Apr 14, 2023

dralley commented Apr 14, 2023 •

edited

Loading

dralley Apr 15, 2023

drahnr Apr 15, 2023

dralley Apr 15, 2023 •

edited

Loading

drahnr Apr 15, 2023

dralley Apr 15, 2023

dralley Apr 15, 2023

dralley Apr 15, 2023 •

edited

Loading

always digest #111

always digest #111

Conversation

drahnr commented Apr 1, 2023 • edited Loading

dralley commented Apr 7, 2023 • edited Loading

drahnr commented Apr 8, 2023

dralley commented Apr 8, 2023

dralley commented Apr 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

cmeister2 left a comment • edited Loading

Choose a reason for hiding this comment

dralley commented Apr 14, 2023 • edited Loading

drahnr commented Apr 14, 2023

dralley commented Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

drahnr commented Apr 1, 2023 •

edited

Loading

dralley commented Apr 7, 2023 •

edited

Loading

cmeister2 left a comment •

edited

Loading

dralley commented Apr 14, 2023 •

edited

Loading

dralley commented Apr 14, 2023 •

edited

Loading

dralley Apr 15, 2023 •

edited

Loading

dralley Apr 15, 2023 •

edited

Loading