Support EdDSA / ed25519 signing and verification #146

dralley · 2023-05-26T23:35:49Z

📜 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-05-27T00:42:27Z

src/rpm/signature/traits.rs

    Self::Signature: AsRef<[u8]>,
 {
    type Signature;
    fn sign<R: Read>(&self, data: R) -> Result<Self::Signature, RPMError>;
+    fn algorithm(&self) -> AlgorithmType;


So here's my thinking:

If we are just handing the Signer / Verifier raw bytes so that it can do the PGP key parsing, then it makes no sense to use a compile-time generic for the algorithm type. It'll figure out the algorithm type when it parses the key, and all keys are just PGP keys so regardless of the algorithm it will be handled by the same codepath, and it can just return an error if the key is of an unsupported type or can't be parsed.

I'm sure we could make this more ergonomic still, but this is as far as I wanted to go in this PR, we can continue cleaning it up later.

Do we really want automatic derivation of the algorithm from the key? That's my key (no pun intended) question here. I'd imagine that a user wants to use a specific key. The compile time guarantees could still be combined.

fn sign_with_key_given(self, key: ..) { let key = key.parse()?; match key.algorithm() { Algorithm::RSA => signer.sign::<Algorithm::RSA>(), // .. }

now this guarantees compiletime consistency and only picks the path according to the convenience route involving algorithm derivation. People that do have the knowledge of the key type statically could maintain correctness by definition.

That's my thinking. Pushing out the runtime checks as far as possible.

Do we really want automatic derivation of the algorithm from the key?

I mean, there's no avoiding that, that's what rpgp does. There's no Signature<Algorithm>, no PublicKey<Algorithm>, these are just types with some details that they know at runtime. So we'd just be creating an artificial difference where none actually exists currently.

That example is still doing automatic derivation of the algorithm of from the key in the exact same way - the key is parsed at runtime, we check the algorithm type to make sure it's an acceptable one, and then sign with the key. The runtime check is happening in exactly the same place, which is first.

The signature creation process isn't the part that differs by algorithm, all that uses the exact same codepath either way (I mean it does different things inside rpgp obviously, but API wise it's the same). It's only how the signatures are added to the header that differs, but the Signer isn't involved in that process, so I don't see how Signer::sign<A>() would provide a real benefit. All Signer does is output a signature from some data provided, it's not attaching the signature anywhere itself.

As a user I have a key, I want to use this key to sign the package, and I can't really think of any reasons to make it more complicated then that. None of the checks are compile-time knowable.

dralley · 2023-05-27T03:27:43Z

@TommyLike

dralley · 2023-05-27T05:02:57Z

src/rpm/package.rs

@@ -148,34 +156,54 @@ impl RPMPackage {
    #[cfg(feature = "signature-meta")]
    pub fn verify_signature<V>(&self, verifier: V) -> Result<(), RPMError>
    where


There are a few open questions:

If you attempt to verify the signatures of a package with no signatures, what error message should it return? RPMError::NoSignatureFound is used for something different (signature packet inspection), but the signature of RPMError::VerificationError is designed in such a way that it can't easily be used outside of Verifier unless we make a few more changes to that API. New error type (e.g. KeyMismatchError, or adjust the existing ones?

Lets say the package is EdDSA signed and I attempt to verify using a Verifier built from an RSA public key. It ought to fail (but currently it doesn't). But again, RPMError::VerificationError can't be used easily outside of Verifier. I suppose that we could force it to try to verify the signature even though we know it will fail, which would raise the error we expect? It's not a terribly satisfying solution, though.

@drahnr Any feedback?

I think I would create a new one and/or re-use RPMError::NoSignatureFound.

I would determine the key type when parsing the key, for most keys that's possible and the just check if there is a signature tag for that key type. Error handling is not pretty if one aims for a good and helpful one: NoSuchSignatureType{ present_signatures: Vec<KeyType> }

drahnr · 2023-05-28T19:19:42Z

src/rpm/builder.rs

@@ -475,7 +475,7 @@ impl RPMBuilder {
    #[cfg(feature = "signature-meta")]
    pub fn build_and_sign<S>(self, signer: S) -> Result<RPMPackage, RPMError>
    where
-        S: signature::Signing<signature::algorithm::RSA>,


I don't quite understand why we revert a compile time check back to a runtime check. Instead of removing it the Algorithm could have become a generic too. The tag id could also be returned, based on if the signature is for header or content, which could be signaled as parameter. That maintains flexibility and extensibility while not having the repetitive sign_with_key_type_{a,b,c}

There was no compile-time check because you don't get to avoid the part where the key itself is of the type you want. With build_and_sign::<algorithm::RSA> you still have to make sure the key you pass in is RSA, except that we would then need to return a runtime error when you pass in a key of the wrong type. So either way you have a runtime check, just with additional boilerplate.

That just doesn't feel helpful at all.

What I tried to say, if we can avoid runtime matches on the key type, and expect the user to provide the correct one, it allows for a more meta code representation approach, which I'd personally prefer over match in quite a few places. The compiler can then enforce a few more things and users that know their key type for sure, can have the match outside of rpm-rs and we can be happy OR possibly take advantage of the type based definition of key types iff they can based on meta programming on their end, too.

Reasons pro generic based crypto:

user can use generics to extend type saftey

compiler help

less match based clutter

avoids the build_and_sign_... variants which all almost do the same

could separate concerns of doing the actual signing vs the mechanics of creating the tag

Imho we should not parse AND sign in the same function, but provide key types that are external to the i.e. build_and_sign call / Signer.

There is no rpgp type that is specific to the algorithm used. No matter what, we have to check the algorithm at runtime, because it is a runtime property of all of the crypto types involved here. We cannot avoid matches on the key type, and we cannot expect the user to pass a correct one - we have to verify that they actually did, and that requires a runtime check.

Further, the code for doing signing is identical and the code for adding signatures and verifying them is 80% identical (and will eventually be identical when we drop some legacy) for both algorithms, so I don't personally like the idea of duplicating the code paths, making maintenance harder and bloating the size of the library.

could separate concerns of doing the actual signing vs the mechanics of creating the tag

Those are already separate though. Signer does the signing, RPMPackage::sign() calls the signing code on the appropriate data regions and adds the signatures to the package.

avoids the build_and_sign_... variants which all almost do the same

We can get rid of that at any time really. Just call build() and then sign() separately. The only reason I haven't done it yet is that IIRC there was a disconnect between passing ownership vs reference.

edit: actually there is maybe a point to keeping build_and_sign() it allows source_date to be reused as the signature timestamp, which removes a step. But the implementations can be merged together.

TommyLike · 2023-06-01T00:37:27Z

@drahnr thanks, looking forward to this feature!

dralley · 2023-06-04T14:11:21Z

@TommyLike You can feel free to review also

src/rpm/headers/signature_builder.rs

dralley · 2023-07-05T15:47:29Z

@drahnr Can I merge this?

TommyLike · 2023-07-05T16:01:14Z

@drahnr Can I merge this?

Support having this. btw, can we verify this signature by rpm command?

dralley · 2023-07-05T18:02:46Z

Yes, it's just a normal RPM signature and rpm --checksigs works with it, it's checkable at installation time, etc.

RPM only supports PGP signing. PGP signatures have a baked in algorithm. So there's no need to bake the algorithm into the API and duplicate a bunch of code when we can just verify at runtime that the parsed PGP signature is one of the appropriate ones, and then do the appropriate thing with it.

closes rpm-rs#126

With probably consolidate later, but not now

And also commit the scripts to recreate them when needed

…ures

CHANGELOG.md

src/rpm/builder.rs

src/rpm/headers/header.rs

src/rpm/signature/pgp.rs

drahnr

A few nits, I am still not particularly excited about the changeset, but I do understand RPMSIGTAG_PGP essentially voids any const generic approaches.

dralley · 2023-07-10T15:45:12Z

Yeah I'm not against any further improvements and we should be able to simplify this further in a year once the SIGTAG_PGP handling can be purged. It ought to be only EL7 which needs it.

dralley requested a review from drahnr May 26, 2023 23:35

dralley force-pushed the eddsa branch 2 times, most recently from 5c6427b to 1310cde Compare May 27, 2023 00:33

dralley changed the title ~~Support EdDSA signing and verification~~ Support EdDSA / ed25519 signing and verification May 27, 2023

dralley commented May 27, 2023

View reviewed changes

dralley marked this pull request as draft May 27, 2023 17:49

dralley force-pushed the eddsa branch 8 times, most recently from f9b748b to b61f14d Compare May 28, 2023 05:35

drahnr reviewed May 28, 2023

View reviewed changes

dralley force-pushed the eddsa branch 2 times, most recently from 847aacd to 1826723 Compare June 1, 2023 00:59

dralley requested a review from drahnr June 2, 2023 14:41

dralley mentioned this pull request Jun 2, 2023

0.12.0 release #153

Merged

TommyLike reviewed Jun 5, 2023

View reviewed changes

src/rpm/headers/signature_builder.rs Show resolved Hide resolved

dralley force-pushed the eddsa branch from 1826723 to ef2d8c2 Compare June 6, 2023 21:16

dralley force-pushed the eddsa branch 3 times, most recently from 0e0fe5a to bbe2288 Compare June 16, 2023 04:22

dralley marked this pull request as ready for review June 16, 2023 04:23

dralley force-pushed the eddsa branch from bbe2288 to 008e7f1 Compare June 16, 2023 04:25

dralley force-pushed the eddsa branch from be9c42b to e109d65 Compare July 5, 2023 15:54

dralley force-pushed the eddsa branch from e109d65 to ef4921f Compare July 6, 2023 21:54

dralley added 8 commits July 9, 2023 04:38

Support EdDSA signing and verification

a424434

closes rpm-rs#126

Move duplicative test

dd35d98

With probably consolidate later, but not now

Delete unused RPM

7a80117

Create new signing keys (RSA and ed25519) to use

3ad9541

And also commit the scripts to recreate them when needed

Add tests for eddsa signing and verification

99c0351

Clean up some formatting

de7338f

Return an error when verifying signatures on a package with no signat…

c72365b

…ures

dralley force-pushed the eddsa branch 4 times, most recently from 5dc5da9 to 4ecf703 Compare July 9, 2023 12:47

Clean up tests

be4dd81

dralley force-pushed the eddsa branch from 4ecf703 to be4dd81 Compare July 9, 2023 12:51

drahnr reviewed Jul 10, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

drahnr reviewed Jul 10, 2023

View reviewed changes

src/rpm/builder.rs Outdated Show resolved Hide resolved

drahnr reviewed Jul 10, 2023

View reviewed changes

src/rpm/headers/header.rs Show resolved Hide resolved

drahnr reviewed Jul 10, 2023

View reviewed changes

src/rpm/signature/pgp.rs Outdated Show resolved Hide resolved

drahnr approved these changes Jul 10, 2023

View reviewed changes

dralley force-pushed the eddsa branch from a088a99 to f27a975 Compare July 10, 2023 15:42

PR review cleanups

1ad06f6

dralley force-pushed the eddsa branch from f27a975 to 1ad06f6 Compare July 10, 2023 15:43

dralley merged commit b6853cf into rpm-rs:master Jul 10, 2023
11 checks passed

dralley deleted the eddsa branch July 10, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support EdDSA / ed25519 signing and verification #146

Support EdDSA / ed25519 signing and verification #146

dralley commented May 26, 2023

dralley May 27, 2023 •

edited

Loading

drahnr May 28, 2023

dralley May 28, 2023 •

edited

Loading

dralley commented May 27, 2023

dralley May 27, 2023 •

edited

Loading

dralley May 31, 2023

drahnr Jun 5, 2023

drahnr Jun 5, 2023

drahnr May 28, 2023 •

edited

Loading

dralley May 28, 2023 •

edited

Loading

drahnr Jun 5, 2023

drahnr Jun 5, 2023

dralley Jun 5, 2023 •

edited

Loading

TommyLike commented Jun 1, 2023

dralley commented Jun 4, 2023

dralley commented Jul 5, 2023

TommyLike commented Jul 5, 2023

dralley commented Jul 5, 2023

drahnr left a comment

dralley commented Jul 10, 2023 •

edited

Loading

Support EdDSA / ed25519 signing and verification #146

Support EdDSA / ed25519 signing and verification #146

Conversation

dralley commented May 26, 2023

📜 Checklist

dralley May 27, 2023 • edited Loading

Choose a reason for hiding this comment

drahnr May 28, 2023

Choose a reason for hiding this comment

dralley May 28, 2023 • edited Loading

Choose a reason for hiding this comment

dralley commented May 27, 2023

dralley May 27, 2023 • edited Loading

Choose a reason for hiding this comment

dralley May 31, 2023

Choose a reason for hiding this comment

drahnr Jun 5, 2023

Choose a reason for hiding this comment

drahnr Jun 5, 2023

Choose a reason for hiding this comment

drahnr May 28, 2023 • edited Loading

Choose a reason for hiding this comment

dralley May 28, 2023 • edited Loading

Choose a reason for hiding this comment

drahnr Jun 5, 2023

Choose a reason for hiding this comment

drahnr Jun 5, 2023

Choose a reason for hiding this comment

dralley Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

TommyLike commented Jun 1, 2023

dralley commented Jun 4, 2023

dralley commented Jul 5, 2023

TommyLike commented Jul 5, 2023

dralley commented Jul 5, 2023

drahnr left a comment

Choose a reason for hiding this comment

dralley commented Jul 10, 2023 • edited Loading

dralley May 27, 2023 •

edited

Loading

dralley May 28, 2023 •

edited

Loading

dralley May 27, 2023 •

edited

Loading

drahnr May 28, 2023 •

edited

Loading

dralley May 28, 2023 •

edited

Loading

dralley Jun 5, 2023 •

edited

Loading

dralley commented Jul 10, 2023 •

edited

Loading