Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Support EdDSA / ed25519 signing and verification #146
Support EdDSA / ed25519 signing and verification #146
Changes from 9 commits
33624d0
a424434
dd35d98
7a80117
3ad9541
99c0351
de7338f
c72365b
be4dd81
1ad06f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 repetitivesign_with_key_type_{a,b,c}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I tried to say, if we can avoid runtime
match
es 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 overmatch
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 thematch
outside ofrpm-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:
match
based clutterbuild_and_sign_...
variants which all almost do the sameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 avoidmatch
es 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.
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.We can get rid of that at any time really. Just call
build()
and thensign()
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 allowssource_date
to be reused as the signature timestamp, which removes a step. But the implementations can be merged together.