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

Support EdDSA / ed25519 signing and verification #146

Merged
merged 10 commits into from
Jul 10, 2023
Merged

Support EdDSA / ed25519 signing and verification #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
33624d0
Remove algorithm dependence in signing/verifying interfaces
dralley May 26, 2023
a424434
Support EdDSA signing and verification
dralley May 26, 2023
dd35d98
Move duplicative test
dralley May 26, 2023
7a80117
Delete unused RPM
dralley Apr 10, 2023
3ad9541
Create new signing keys (RSA and ed25519) to use
dralley May 19, 2023
99c0351
Add tests for eddsa signing and verification
dralley May 26, 2023
de7338f
Clean up some formatting
dralley Jun 1, 2023
c72365b
Return an error when verifying signatures on a package with no signat…
dralley May 27, 2023
be4dd81
Clean up tests
dralley Jun 15, 2023
1ad06f6
PR review cleanups
dralley Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,38 @@
# Changelog

All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## Breaking Change
### Breaking Change

- Removed `RPM` prefix from type names, e.g. `RPMPackage` is renamed to `Package`.
- `RPMBuilder` is renamed to `PackageBuilder`.
- The `PackageBuilder::build_time` method is removed. Package build time is now
included by default and can be clamped using the `PackageBuilder::source_date` method.
- Several of the signer and verifier trait APIs were changed

## Added
### Added

- `PackageBuilder::source_date` method for clamping modification time of files,
build time of the package, and signature time. This functionality is required for
build time of the package, and signature timestamp. This functionality is required for
reproducible generation of packages.
- `Package::sign_with_timestamp` method.
- `Package::sign_with_timestamp` method for signing a package while using a specific
timestamp. This is needed to reproducibly sign packages.
- `PackageMetadata::signature_key_id` method for getting the signing key ID (superset
of the fingerprint) of the key used to sign a package as a hex-encoded string.
Key fingerprints can be easily extracted from this value.
- The "rpmversion" tag is now populated so that packages know which library (and version)
they were built with.
- Support for signing and verification with EdDSA signatures

### Changed

- Build time metadata is now included in the built package by default
- The algorithm type is no longer baked into the Signing and Verifying APIs as it is unnecessary.

### Fixed

Expand Down
4 changes: 4 additions & 0 deletions src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ pub enum Error {
#[error("unsupported digest algorithm {0:?}")]
UnsupportedDigestAlgorithm(DigestAlgorithm),

#[cfg(feature = "signature-pgp")]
#[error("unsupported PGP key type {0:?}")]
UnsupportedPGPKeyType(pgp::crypto::public_key::PublicKeyAlgorithm),

#[error("invalid file mode {raw_mode} - {reason}")]
InvalidFileMode { raw_mode: i32, reason: &'static str },

Expand Down
36 changes: 20 additions & 16 deletions src/rpm/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ impl PackageBuilder {
#[cfg(feature = "signature-meta")]
pub fn build_and_sign<S>(self, signer: S) -> Result<Package, Error>
where
S: signature::Signing<signature::algorithm::RSA>,
Copy link
Contributor

@drahnr drahnr May 28, 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 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}

Copy link
Collaborator Author

@dralley dralley May 28, 2023 •
edited
Loading

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.

Copy link
Contributor

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 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

Copy link
Contributor

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.

Copy link
Collaborator Author

@dralley dralley Jun 5, 2023 •
edited
Loading

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 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.

S: signature::Signing,
{
let source_date = self.source_date;
let (lead, header_idx_tag, content) = self.prepare_data()?;
Expand All @@ -516,31 +516,35 @@ impl PackageBuilder {
header_digest_sha256,
} = Package::create_sig_header_digests(header.as_slice(), content.as_slice())?;

let now = Timestamp::now();
let signature_timestamp = match source_date {
Some(source_date_epoch) if source_date_epoch < now => source_date_epoch,
_ => now,
};

let builder = Header::<IndexSignatureTag>::builder().add_digest(
header_digest_sha1.as_str(),
header_digest_sha256.as_str(),
header_and_content_digest_md5.as_slice(),
);

let signature_header = {
let now = Timestamp::now();
let t = match source_date {
Some(sde) if sde < now => sde,
_ => now,
};
let rsa_sig_header_only = signer.sign(header.as_slice(), t)?;
let sig_header_only = signer.sign(header.as_slice(), signature_timestamp)?;

let cursor = io::Cursor::new(header).chain(io::Cursor::new(&content));
let rsa_sig_header_and_archive = signer.sign(cursor, t)?;
let builder = match signer.algorithm() {
signature::AlgorithmType::RSA => {
let mut header_and_content_cursor =
io::Cursor::new(header.as_slice()).chain(io::Cursor::new(content.as_slice()));

builder
.add_signature(
rsa_sig_header_only.as_ref(),
rsa_sig_header_and_archive.as_ref(),
)
.build(header_and_content_len)
let sig_header_and_archive =
signer.sign(&mut header_and_content_cursor, signature_timestamp)?;
builder.add_rsa_signature(sig_header_only.as_ref(), sig_header_and_archive.as_ref())
}
signature::AlgorithmType::EdDSA => {
builder.add_eddsa_signature(sig_header_only.as_ref())
}
};

let signature_header = builder.build(header_and_content_len);
let metadata = PackageMetadata {
lead,
signature: signature_header,
Expand Down
89 changes: 6 additions & 83 deletions src/rpm/headers/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ where
Ok(())
}

pub fn entry_is_present(&self, tag: T) -> bool {
self.index_entries
.iter()
.any(|entry| entry.tag == tag.to_u32())
}

pub(crate) fn find_entry_or_err(&self, tag: T) -> Result<&IndexEntry<T>, Error> {
self.index_entries
.iter()
Expand Down Expand Up @@ -319,28 +325,6 @@ impl fmt::Display for Header<IndexTag> {
}

impl Header<IndexSignatureTag> {
/// Create a new full signature header.
///
/// `size` is combined size of header, header store and the payload
///
/// PGP and RSA tags expect signatures according to [RFC2440](https://tools.ietf.org/html/rfc2440)
///
/// Please use the [`builder`](Self::builder()) which has modular and safe API.
#[cfg(feature = "signature-meta")]
pub(crate) fn new_signature_header(
headers_plus_payload_size: usize,
md5sum: &[u8],
sha1: &str,
sha256: &str,
rsa_spanning_header: &[u8],
rsa_spanning_header_and_archive: &[u8],
) -> Self {
SignatureHeaderBuilder::new()
.add_digest(sha1, sha256, md5sum)
.add_signature(rsa_spanning_header, rsa_spanning_header_and_archive)
.build(headers_plus_payload_size)
}

pub fn builder() -> SignatureHeaderBuilder<Empty> {
SignatureHeaderBuilder::<Empty>::new()
}
Expand Down Expand Up @@ -600,7 +584,6 @@ impl<T: Tag> IndexEntry<T> {
}

pub(crate) fn write_index(&self, out: &mut impl std::io::Write) -> Result<(), Error> {
// unwrap() is safe because tags are predefined.
let mut written = out.write(&self.tag.to_be_bytes())?;
written += out.write(&self.data.type_as_u32().to_be_bytes())?;
written += out.write(&self.offset.to_be_bytes())?;
Expand Down Expand Up @@ -906,64 +889,4 @@ mod test {

Ok(())
}

#[cfg(feature = "signature-meta")]
#[test]
fn signature_header_build() {
drahnr marked this conversation as resolved.
Show resolved Hide resolved
let size: u32 = 209_348;
let md5sum: &[u8] = &[22u8; 16];
let sha1 = "5A884F0CB41EC3DA6D6E7FC2F6AB9DECA8826E8D";
let sha256 = "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855";
let rsa_spanning_header: &[u8] = b"111222333444";
let rsa_spanning_header_and_archive: &[u8] = b"7777888899990000";

let truth = {
let offset = 0;
let entries = vec![
IndexEntry::new(
IndexSignatureTag::RPMSIGTAG_SIZE,
offset,
IndexData::Int32(vec![size]),
),
// TODO consider dropping md5 in favour of sha256
IndexEntry::new(
IndexSignatureTag::RPMSIGTAG_MD5,
offset,
IndexData::Bin(md5sum.to_vec()),
),
IndexEntry::new(
IndexSignatureTag::RPMSIGTAG_SHA1,
offset,
IndexData::StringTag(sha1.to_owned()),
),
IndexEntry::new(
IndexSignatureTag::RPMSIGTAG_SHA256,
offset,
IndexData::StringTag(sha256.to_owned()),
),
IndexEntry::new(
IndexSignatureTag::RPMSIGTAG_RSA,
offset,
IndexData::Bin(rsa_spanning_header.to_vec()),
),
IndexEntry::new(
IndexSignatureTag::RPMSIGTAG_PGP,
offset,
IndexData::Bin(rsa_spanning_header_and_archive.to_vec()),
),
];
Header::<IndexSignatureTag>::from_entries(entries, IndexSignatureTag::HEADER_SIGNATURES)
};

let built = Header::<IndexSignatureTag>::new_signature_header(
size as usize,
md5sum,
sha1,
sha256,
rsa_spanning_header,
rsa_spanning_header_and_archive,
);

assert_eq!(built, truth);
}
}
Loading