signing

[drahnr]

The commit history still is a mess, this is just to show the outline of what I'd like to implement.

Note that I also move things around a bit so it's not one giant file with 10k loc :)

[Richterrettich]

Would it be possible to open a separate PR for the refactoring part? I like it, but it is far easier to review and discuss two focuesed (somewhat) smaller PR's than one big patch. This would help me a lot :)

[drahnr]

Yep, that was the plan for later today (sorry for the mess )

[Richterrettich]

I guess you have to rebase now after the merge of #5 :)

[drahnr]

Which reference did you use for your implementation? Just to make sure we're on the same page :)

[Richterrettich]

I mainly used this: http://ftp.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html and this: https://xyrillian.de/thoughts/posts/argh-pm.html

As well as some hints from the FPM source code and reverse engineering since the original C source code is very confusing.

[Richterrettich]

What is most important to me is correctness. RPM is a messy file format and we need a good test harness for this feature.

I think we should create a container based test case where we sign some packages with rpm directly. Afterwards, we create the same package with rpm-rs and sign it with your implementation. We can then compare the two and hopefully get the exact same rpm.

[drahnr]

What is most important to me is correctness. RPM is a messy file format and we need a good test harness for this feature.

Frankly, I did not expect the rpm fmt to be that kind of a mess, combined with a lot impl detail limitation when it comes to cryptographic choices. From the RPM 4.15.1 sources https://rpm.org/download.html, I saw that currently only RSA is supported. I'll update this PR soon to add a minimal set of deps which allows key imports without pulling in openssl as a dependency.

I think we should create a container based test case where we sign some packages with rpm directly. Afterwards, we create the same package with rpm-rs and sign it with your implementation. We can then compare the two and hopefully get the exact same rpm.

Setting up CI is a non-issue. I'd recommend to cross check rpm --addsign $pkg and rpm --checksign $pkg against the rust impl.

[drahnr]

The latest commit just outlines a set of crates which allow signing without pulling in openssl or some other giant TLS library - for RSA key loading in PEM and PKCS#8/DER fmt https://github.com/caelunshun/rsa-der/pull/3 should be completed.

[Richterrettich]

Frankly, I did not expect the rpm fmt to be that kind of a mess, combined with a lot impl detail limitation when it comes to cryptographic choices.

Yeah, it is a mess in many regards.

I just had a quick look and your changes look quite interresting. I need to look deeper into it since I am not an crypto expert tbh.

[drahnr]

I followed rfcs and the unit tests cross check with openssl, so far no incompatibilities were uncovered.

My plan is to wrap the changes up comple the impl.

[drahnr]

Good old compiling state, there is still one test failing I have yet to look into that and do some additional integration tests with a signed dummy rpm.

I'd love to hear some feedback and it'd be awesome if you could take a look at the few spots which were not entirely clear to me how to handle things regarding lengths.

[drahnr]

The only thing left now is verifying everything is passing dnf and rpm signature validations

[Richterrettich]

I'll have a look in the upcoming days

[drahnr]

Maybe next week I can find a bit of time to implement some tests covering signatures, not sure if it makes sense to review before that.

[drahnr]

So as it seems, the sign verify cycle is currently broken which in turn breaks signin, I am not yet 100% sure where the broken primes come from. Given the rsa* secondary crates are seemingly mostly unmaintained, it might make sense to pull in something like ring, but extract all crypto in crate local types so it can be replaced easily (or extended, if that will ever be done :man_shrugging: )

I'd still like some review comments at some point :)

[Richterrettich]

it might make sense to pull in something like ring, but extract all crypto in crate local types

I think this would be the best solution indeed. Ring looks promising and seems to have some traction.

I'd still like some review comments at some point :)

Do you want to restructure the crate first with the mentioned changes including ring? I can do a review of the user facing API this weekend if you wish though.

[drahnr]

it might make sense to pull in something like ring, but extract all crypto in crate local types

I think this would be the best solution indeed. Ring looks promising and seems to have some traction.

I'll add some trait abstractions then and implement signing using a default impl using ring, that also allows other people to implement their own signing backend at will (such as a yubikey/yubihsm based signer).

I'd still like some review comments at some point :)

Do you want to restructure the crate first with the mentioned changes including ring? I can do a review of the user facing API this weekend if you wish though.

The one thing I would really appreciate a second pair of eyes on is the part where the byte ranges for the signatures are selected. I think everything (besides the inner workings of Signer/Verifier) are ready for review :)

[Richterrettich]

Did I miss any more byte range / offset problems you need help with? The rest looks good to me.

[drahnr]

No I think it was only those two spots, thanks!

[Richterrettich]

One thing we have keep in mind when pulling a third party crypto crate is to check license compatibility. Ring's license seems compatible with apache2 so it is fine.

[drahnr]

I don't have a preference right now, with the trait based approach, RPMPackage will become a generic over Signer/Verifeer associated type, so technically rpm-rs would not necessarily implement ANY crypto at all and push that into a separate crates - I would still do a ring based impl as default, and avoid the extra hassle.

[drahnr]

The API turned out to be a lot simpler than anticipated.

---

ring is not going to work, it ties the digest into the signing method, which lacks md5.

crypto impl based on pgp+rsa or rustls seem feasible - they both provide a direct signing API.

[drahnr]

    Header RSA signature: BAD (package tag 268: invalid OpenPGP signature)
    Header SHA1 digest: OK
    RSA signature: BAD (package tag 1002: invalid OpenPGP signature)
    MD5 digest: OK

currently signing fails with both, pgp and ring impl with the above issues, I have yet to investigate what's going wrong here.

[drahnr]

The API turned out to be a lot simpler than anticipated.

ring is not going to work, it ties the digest into the signing method, which lacks md5.

crypto impl based on pgp+rsa or rustls seem feasible - they both provide a direct signing API.

Partially wrong, the signatures only need to be compatible with sha2_256 as far as I can tell, which works well with ring.

[drahnr]

@Richterrettich as of https://github.com/Richterrettich/rpm-rs/pull/4/commits/027e5a5ea19ce3616f611ff4d779eac9b9fe5fda with signing-pgp signatures work and verified both ways against the rpm binary.

The plan would be to invest very little time into getting the rfc4880 straightend out and implementing some more default key parsing features plus a badly needed round of cleanups.

[Richterrettich]

nice! yeah, I am looking forward to it. Just tell me if I should have a look at something

[drahnr]

I'll ping you again once the cleanup is complete :)

• [ ] ~investigate effort for ring based signing impl~ not happening, crate pgp has all we need and is security audited
• [ ] ~impl DER format parsing for signing-pgp feature~ is not going to be implemented within this PR, possibly at a later point of time
• [x] impl PEM format parsing for signing-pgp feature - a default is implemented based on the pem crate, but unwraps the format and relies on the impl of a DER parser (either PKCS#1 or PKCS#8 compliant)
• [x] remove stale dependencies

[drahnr]

So after a way too short night and reading way too many rfc's and tickets about rpm and it's issues (past & present) there is one remaining issue:

rpm signs with a subkey.

But neither the secret nor the public key contain a key that verifies the signature, while it parses just fine.

[stdout] gpg: using subkey 77500CC056DB3521 instead of primary key CFD331925AB27F39
[stdout] gpg: writing to '/out/389-ds-base-devel-1.3.8.4-15.el7.x86_64.rpm.sig'
[stdout] gpg: RSA/SHA256 signature from: "77500CC056DB3521 Package Manager (unprotected) <pmanager@example.com>"
[stdout] gpg: using subkey 77500CC056DB3521 instead of primary key CFD331925AB27F39
[stdout] gpg: writing to '/out/389-ds-base-devel-1.3.8.4-15.el7.x86_64.rpm.sig'
[stdout] gpg: RSA/SHA256 signature from: "77500CC056DB3521 Package Manager (unprotected) <pmanager@example.com>"
[stdout] /out/389-ds-base-devel-1.3.8.4-15.el7.x86_64.rpm:
[stdout] /out/389-ds-base-devel-1.3.8.4-15.el7.x86_64.rpm:
[stdout]     Header V4 RSA/SHA256 Signature, key ID 56db3521: OK
[stdout]     Header SHA1 digest: OK
[stdout]     V4 RSA/SHA256 Signature, key ID 56db3521: OK
[stdout]     MD5 digest: OK

I'd be more than happy for any hints what is going on, or as first step, to force gpg to not sign with subkeys.

Other than that, it's ready to merge.

[drahnr]

So now it becomes more obscure:

I added this tests which shows that the signature created is not one of the keys in the public or private key (or at least, doesn't have a know key signature) https://github.com/Richterrettich/rpm-rs/pull/4/commits/786989bf25c407fb2378b8bc874047534f0248eb#diff-af3d323a174d88bd942ccae59856fc94R52-R91

~If you have any idea / hint what might be going on here @Richterrettich that would be much appreciated :)~

gpg: Signature made So 19 Apr 2020 17:59:42 CEST
gpg:                using RSA key 159F7EF2261D79517311525F77500CC056DB3521
gpg:                issuer "pmanager@example.com"
gpg: BAD signature from "Package Manager (unprotected) <pmanager@example.com>" [unknown]

~where both of them are imported and it works with rpm --addsign / --verify~

Resolved as part of https://github.com/Richterrettich/rpm-rs/pull/4/commits/a037b0de754c1a7943a7148ff2930944e7f84aad - last but not least get rpm to work based on that commit.

[drahnr]

@Richterrettich this is ready for review, all tests passed :tada:

A couple of notes:

• container based tests are executed in sequence now, which avoids cache collisions for the dnf package caches - caching is needed since syncing the pkg index as well as downloading rpmsign's dependencies was root of ~95% of the test execution time
• quite a few tests verify that the host installed gpg works as anticipated as well as various checks for the pgp crate

[Richterrettich]

Okay, I'll have a look in the upcoming days.

[Richterrettich]

After the final API changes are in place and my internal testing is positive we can wrap this up and merge it.

[drahnr]

Very much looking forward! Changes are pushed, plus a blanket impl for references of Signer also implementing Signer respective impl for Verifying

[Richterrettich]

So if we only pass signers/verifiers around, do you think we still need the KeyLoader trait? It seems like we should remove it and leave it to the signer/verifier implementation to properly load those keys. What do you think?

[drahnr]

I had the same gut feeling, it's removed now.

[Richterrettich]

My tests are all positive. So it is looking good from a functional point of view too.

[drahnr]

I think I addressed all comments

[Richterrettich]

Yes, this looks good to me now. Thank you for your hard work on this :)