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.

Sign up for GitHub

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

Jump to bottom

Create & verify PAYLOADDIGEST, PAYLOADDIGESTALT #124

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Create & verify PAYLOADDIGEST, PAYLOADDIGESTALT #124

merged 4 commits into from
Apr 22, 2023

Conversation

dralley
Copy link
Collaborator

@dralley dralley commented Apr 15, 2023 •
edited
Loading

📜 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 marked this pull request as draft April 15, 2023 19:12
@dralley dralley force-pushed the payloaddigest branch 3 times, most recently from 4a98ae5 to 8ffcdfe Compare April 15, 2023 19:47
@dralley
Copy link
Collaborator Author

dralley commented Apr 16, 2023 •
edited
Loading

@DemiMarie (edit: see https://github.com/rpm-software-management/rpm/issues/2486)

@dralley dralley force-pushed the payloaddigest branch 2 times, most recently from 5a05a88 to 0652df2 Compare April 16, 2023 19:15
@dralley dralley marked this pull request as ready for review April 16, 2023 19:16
@dralley dralley requested a review from drahnr April 16, 2023 19:16
@dralley dralley force-pushed the payloaddigest branch 2 times, most recently from a378e8a to ff1c21a Compare April 16, 2023 19:18
@dralley dralley mentioned this pull request Apr 16, 2023
Closed
@dralley dralley force-pushed the payloaddigest branch 4 times, most recently from 8867997 to b5d8408 Compare April 16, 2023 23:35
dralley
dralley commented Apr 17, 2023
View reviewed changes
drahnr and others added 3 commits April 16, 2023 23:58
@drahnr @dralley
revert: remove inconsistent use of outcome enums, add another error v…
75c832c 
…ariant instead
@dralley
Generate PAYLOADDIGEST, PAYLOADDIGESTALT, PAYLOADDIGESTALGO
dceecea 
These are checksums of the compressed payload, uncompressed payload, and
the algorithm used respectively.
@dralley
Streaming digest calculation of raw archive
0af74e9
drahnr
drahnr reviewed Apr 17, 2023
View reviewed changes
src/rpm/package.rs
@@ -216,31 +208,59 @@ impl RPMPackage {
);

verifier.verify(header_bytes.as_slice(), signature_header_only)?;
self.verify_digests()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended to always verify digests? If they're missing, the verification would fail and the signatures themselves would already contain the content.

I'd be in favor of providing a on-stop-shop fn verify that verifies whatever is in the package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that if you just verify the signature without verifying the digests you're not actually asserting that the payload is intact (at least not in all cases, if you have only modern signatures and not the legacy ones)

We'll probably need to rework these APIs in the future, in order to support EdDSA signatures in addition to RSA. Currently RSA is baked into the signature of this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, but we do verify both, at least that's what I remember.

Copy link
Collaborator Author

@dralley dralley Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so, we verify old signatures and new signatures, but that's on a different dimension from the actual type of signatures. There's RSAHEADER and DSAHEADER, but we don't have any code that reads DSAHEADER. DSAHEADER is where EdDSA signatures are stored (it was repurposed as original DSA support has been dead for a while)

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've change the implementation to only verify digests that actually exist.

drahnr
drahnr reviewed Apr 17, 2023
View reviewed changes
drahnr
drahnr requested changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few nits and details to be clarified, other than that, lgtm

@dralley dralley requested a review from drahnr April 17, 2023 14:47
@dralley dralley force-pushed the payloaddigest branch 3 times, most recently from 630c1bc to 840b09f Compare April 17, 2023 15:04
drahnr
drahnr reviewed Apr 18, 2023
View reviewed changes
drahnr
drahnr reviewed Apr 18, 2023
View reviewed changes
src/rpm/builder.rs
@@ -675,7 +682,7 @@ impl RPMBuilder {
"4.0-1".to_string(),
));

if matches!(self.compressor, Compressor::Zstd(_)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we impl fn as_dependency(&self) -> Option<Dependency> for the compressor we can simply call self.requires.extend(compressor.as_dependency()) which is much cleaner.

Copy link
Collaborator Author

@dralley dralley Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure that pattern would generalize well enough to be worthwhile. In fact this might be the only case where that is possible. With FileDigests or RichDeps or any other feature requires there isn't a particular object you could attach it to.

drahnr
drahnr requested changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few idiomatic adjustments, other than that, 👍

@dralley dralley requested a review from drahnr April 18, 2023 16:48
@dralley
Copy link
Collaborator Author

dralley commented Apr 21, 2023

@drahnr Completely unrelated question. I have an implementation of RPM version comparison which I would like to upload. I could add it to this project, or we could make a new crate for it. I kind of prefer he latter.

  1. Do you think that makes sense?
  2. If so, can I have permissions to create new repos in the org or transfer repos into the org?

@drahnr
Copy link
Contributor

drahnr commented Apr 22, 2023

If possible, make it a separate crate, add it to this repository, add a workspace entry to the Cargo.toml. That's imho the least churn/value.

@dralley dralley merged commit 542166e into rpm-rs:master Apr 22, 2023
@dralley dralley deleted the payloaddigest branch April 22, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drahnr drahnr drahnr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dralley @drahnr