Always create package digests #106

dralley · 2023-03-16T02:05:33Z

Remove the signature-meta feature because it's a bit superfluous

closes #105

📜 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

src/compat_tests.rs

drahnr

See the inline comment. I think we should decouple signature creation from the signing. Hence I don't think combining pgp with the API of signing/verification is the right approach.

dralley · 2023-03-17T00:49:33Z

@drahnr Take another look, I removed [cfg(feature = "signature-pgp")] from everything not explicitly touching pgp

Remove the signature-meta feature because it's a bit superfluous closes rpm-rs#105

drahnr

It's not in going into the direction I was thinking. Why change all the feature flags regarding signatures? I'd have expected splitting the hash/checksum creation from the whole signature creation logic, which does not need any changes on the feature flags side.

drahnr · 2023-03-19T18:59:43Z

I'll comment in more detail at some point in the next few days

dralley · 2023-03-27T21:05:19Z

@drahnr ^

drahnr · 2023-03-28T05:54:37Z

src/rpm/builder.rs

@@ -389,7 +387,6 @@ impl RPMBuilder {
        let mut header = Vec::with_capacity(128);
        header_idx_tag.write(&mut header)?;

-        #[cfg(feature = "signature-meta")]


Yes, we should always build this, so removing the signature-meta tag here is correct.

drahnr · 2023-03-28T05:56:18Z

src/rpm/builder.rs

@@ -422,7 +417,6 @@ impl RPMBuilder {
    /// use an external signer to sing and build
    ///
    /// See `signature::Signing` for more details.
-    #[cfg(feature = "signature-meta")]


This is where I disagree. build_and_sign is user visible API and should not be usable for skipping signatures. Imho build should also create the header, but not sign it.

This is where I disagree. build_and_sign is user visible API and should not be usable for skipping signatures.

What do you mean by "not be usable for skipping signatures"?

My thought process is this:

It's possible for users to sign packages without using signature-pgp, because they can create their own implementer of the Signing trait

It's not possible for users to use this API without providing an implementor of the Signing trait because it's a non-optional argument - it's not a step they can skip

signature-meta doesn't provide much value because the amount of code you skip by putting it behind a feature is minimal. For most users they will want the concrete implementations in signature-pgp, but there's not really harm in having functions which use those generic APIs be available always, because it's conceivable that a user could still use them.

I guess I don't feel that strongly about it, so if you do strongly feel that signature-meta should be kept then we can keep it. I just don't see it as having much practical value separate from conceptual structural value.

Imho build should also create the header, but not sign it.

That's what it currently does (in this PR)? It produces a signature header and populates it with digests, but not the signatures.

https://github.com/rpm-rs/rpm/pull/106/files#diff-b82882f602be2cf2d09fd6546c2097235d9417da83223e9516b8eb783c6257ffR396

On a separate note though, we could almost do away with build_and_sign() if we take RPMPackage::sign() and make it take and return self instead of &mut self and ().

That way you could write this chain, which ought to be equivalent to build_and_sign().

let pkg = RpmBuilder::new(....) [....] .build() .sign(signer);

Or at least reimplement the former in terms of the latter to avoid the amount of code duplication we currently have between the 3 functions. Though we could address that in other ways, too.

drahnr · 2023-03-28T05:56:30Z

src/rpm/builder.rs

@@ -471,7 +465,6 @@ impl RPMBuilder {
    }

    /// use prepared data but make sure the signatures are
-    #[cfg(feature = "signature-meta")]


drahnr · 2023-03-28T05:57:46Z

src/rpm/headers/header.rs

@@ -163,7 +163,7 @@ where
            .ok_or_else(|| RPMError::TagNotFound(tag.to_string()))
    }

-    #[cfg(feature = "signature-meta")]
+    #[allow(unused)]


It shouldn't be unused, either it's required in meta or any of the the given concrete signer implementations which should end up in a cfg(any(..)) annotation or be deleted.

The reason it's marked allow(unused) is that it's marked pub(crate) and there are no internal users except for the signature verification code, which isn't always enabled. So we can't outright delete it.

However, this is pretty generic functionality (not actually feature-specific), and I can't think of a great reason to not allow reading arbitrary tags so long as we make it clear that it's not the preferred way when methods exist on RpmPackageMetadata. And I imagine we won't want to write specific functions for every single possible tag including the obscure ones that some crazy person might still want to read.

So probably we should just make the methods for reading header tags public.

drahnr · 2023-03-28T05:58:57Z

src/tests.rs

@@ -31,7 +31,7 @@ fn cargo_manifest_dir() -> std::path::PathBuf {
    std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR"))
 }

-#[cfg(feature = "signature-meta")]
+#[cfg(feature = "signature-pgp")]


Doesn't require the pgp signer, the API is enough.

Correct 👍

dralley · 2023-03-31T18:01:53Z

@drahnr How should I proceed? Would you like signature-meta returned to the few places noted or did my explanation make sense?

drahnr · 2023-03-31T18:07:59Z

As you can imagine from my latency, I didn't get around to suggest a viable path, but I do have an intuition what needs to be done. I hope I can create a draft PR really soon

drahnr · 2023-04-14T14:37:18Z

Closing, #111 is merged and covers the same functionality, but does not interleave it with the signature logic but detaches it.

dralley requested a review from drahnr March 16, 2023 02:05

dralley force-pushed the payloaddigest branch 4 times, most recently from c4c67e0 to 9f0a3b2 Compare March 16, 2023 02:23

drahnr reviewed Mar 16, 2023

View reviewed changes

src/compat_tests.rs Show resolved Hide resolved

drahnr requested changes Mar 16, 2023

View reviewed changes

dralley force-pushed the payloaddigest branch 2 times, most recently from ccd43ab to 10924b5 Compare March 17, 2023 00:47

dralley requested a review from drahnr March 17, 2023 00:49

Always create package digests

7937cd4

Remove the signature-meta feature because it's a bit superfluous closes rpm-rs#105

dralley force-pushed the payloaddigest branch from 10924b5 to 7937cd4 Compare March 19, 2023 17:09

drahnr requested changes Mar 19, 2023

View reviewed changes

drahnr requested changes Mar 28, 2023

View reviewed changes

drahnr mentioned this pull request Apr 1, 2023

always digest #111

Merged

drahnr closed this Apr 14, 2023

dralley deleted the payloaddigest branch April 15, 2023 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always create package digests #106

Always create package digests #106

dralley commented Mar 16, 2023 •

edited

Loading

drahnr left a comment

dralley commented Mar 17, 2023

drahnr left a comment

drahnr commented Mar 19, 2023

dralley commented Mar 27, 2023

drahnr Mar 28, 2023

drahnr Mar 28, 2023

dralley Mar 28, 2023 •

edited

Loading

dralley Mar 29, 2023 •

edited

Loading

drahnr Mar 28, 2023

drahnr Mar 28, 2023

dralley Mar 28, 2023 •

edited

Loading

drahnr Mar 28, 2023

dralley Mar 31, 2023

dralley commented Mar 31, 2023

drahnr commented Mar 31, 2023

drahnr commented Apr 14, 2023

Always create package digests #106

Always create package digests #106

Conversation

dralley commented Mar 16, 2023 • edited Loading

📜 Checklist

drahnr left a comment

Choose a reason for hiding this comment

dralley commented Mar 17, 2023

drahnr left a comment

Choose a reason for hiding this comment

drahnr commented Mar 19, 2023

dralley commented Mar 27, 2023

drahnr Mar 28, 2023

Choose a reason for hiding this comment

drahnr Mar 28, 2023

Choose a reason for hiding this comment

dralley Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

dralley Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

drahnr Mar 28, 2023

Choose a reason for hiding this comment

drahnr Mar 28, 2023

Choose a reason for hiding this comment

dralley Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

drahnr Mar 28, 2023

Choose a reason for hiding this comment

dralley Mar 31, 2023

Choose a reason for hiding this comment

dralley commented Mar 31, 2023

drahnr commented Mar 31, 2023

drahnr commented Apr 14, 2023

dralley commented Mar 16, 2023 •

edited

Loading

dralley Mar 28, 2023 •

edited

Loading

dralley Mar 29, 2023 •

edited

Loading

dralley Mar 28, 2023 •

edited

Loading