rpm-software-management / rpm

The RPM package manager
http://rpm.org
Other
506 stars 364 forks source link

Provide a decent API for verifying package signatures #2041

Open DemiMarie opened 2 years ago

DemiMarie commented 2 years ago

Currently, there is no decent API to verify package signatures. There are various APIs that can be used, but they all are flawed in one way or another.

I propose an rpmRC rpmVerifyPackageSignature(rpmContext *context, int fd, uint64_t flags); API that just does the right thing for normal RPMs, together with an rpmRC rpmVerifyDeltaPackageSignature(rpmContext *context, int fd, uint64_t flags); that does the right thing for delta RPMs. The difference is that in the second case, the header+payload signature is required, and the payload digest is ignored as it will always be wrong.

The context is meant to handle stuff like logging and other state that is currently global.

Some of the flags I can think of:

pmatilai commented 2 years ago

Yup, a sane signature verification API is needed, it was always part of the plan when adding the rpmvs.* stuff. The problem is finding the time + energy of sitting down and designing a sane one that actually covers the needs now and sufficiently flexible for various future needs too. Short of that, I've considered exporting something close to rpmpkgVerifySigs() (minus the logging basically). It has it's shortcomings but as The Good API refuses to stand up...

It'd be useful to list the various flaws with the existing APIs, from someone trying to deal with it as an external API user. The existing stuff covers the needs of rpm sufficiently, but life on the outside is always very different. Been there, but long forgotten. For that reason it's also important that whatever is added is used by rpm itself too.

I disagree with the delta rpm API though. Header+payload signatures are very much on their way out, and nothing new should rely on them. Deltarpm tries to create bit-per-bit compatible payload, and most of the time succeeds, payload digest isn't any more wrong than a header+payload signature would be. However deltarpm should just start creating an uncompressed payload which allows payloaddigestalt to be used instead (that's it's primary use-case) and once we've phased out header+payload signatures (and digests), there's no need for any other magic wrt deltarpm.

DemiMarie commented 2 years ago

I disagree with the delta rpm API though. Header+payload signatures are very much on their way out, and nothing new should rely on them. Deltarpm tries to create bit-per-bit compatible payload, and most of the time succeeds, payload digest isn't any more wrong than a header+payload signature would be. However deltarpm should just start creating an uncompressed payload which allows payloaddigestalt to be used instead (that's it's primary use-case) and once we've phased out header+payload signatures (and digests), there's no need for any other magic wrt deltarpm.

The problem with this is that it requires deltarpms to be applied before any signatures can be checked. This is a very bad idea: it means that any code execution bug in the tools responsible for applying deltarpms becomes a remote root hole. The header+payload signature allows for v3 deltarpms to be verified before having to parse them, which is much, much safer. More generally, signature verification needs to come first; everything else (such as decompression, applying delta instructions, etc) should come afterwards, when the input is no longer untrusted. Since deltarpms are small, most of the drawbacks of the header+payload signature do not apply to them. The payload digest cannot be used because it will always be incorrect, and while payloaddigestalt is nice to check, it cannot be checked soon enough.

It'd be useful to list the various flaws with the existing APIs, from someone trying to deal with it as an external API user. The existing stuff covers the needs of rpm sufficiently, but life on the outside is always very different. Been there, but long forgotten. For that reason it's also important that whatever is added is used by rpm itself too.

Will do, eventually, once I get the time. Whenever I had to verify a package signature, I either invoked the rpmkeys binary or (as in the case of rpmcanon) did everything myself except for crypto.

DemiMarie commented 2 years ago

To elaborate: v3 deltarpms contain their own signature header, separate from the signature header of the reconstructed RPM. This means that a header+payload signature of a v3 deltarpm can actually be valid, as can the header signature. The payload digest will obviously be wrong, though. I believe this is one of the motivations for the v3 deltarpm format, and @Conan-Kudo and I have both expressed interest in using this feature.

pmatilai commented 2 years ago

I have no clue about any deltarpm versions or plans in that area. Rpm will stop processing header+payload signatures and digests in foreseeable future though, so if deltarpm wants to verify such things it'll need to do it by itself.

nwalfield commented 2 years ago

Is this issue the right place to talk about an updated API for the PGP backend, as mentioned elsewhere, or would it be better to open a different issue for that?

Actually, I have no clue what a good API for this would look like smile So if you do, by all means propose something like a rough outline and we can go from there. I guess I mostly care that it doesn't look too outlandish in the rpm codebase, but considering what an hodge-podge the rpm API is, minor deviances get lost in the noise.

pmatilai commented 2 years ago

It's at least closely related, so I don't see any harm discussing it here. On that note, DNF is interested in this all too, but their needs go beyond package signatures. @j-mracek can tell more about that.

j-mracek commented 2 years ago

Thank you for ping. DNF team is looking for a replacement of GPGME for verification of signed metadat in repositories, therefore if there will be an option to extend RPM capability to also verify alternative file using keys not only from internal RPMDB keyring it would be great and everyone will benefit from such a solution or we will share the same pain. It also means that DNF would like to help with solution development and support. Of course there are some limitation - it will be difficult to accept a solution that will enlarge a minimal image.

pmatilai commented 2 years ago

FWIW, you can already use a custom keyring populated from whatever source you like. Similarly it should be entirely possible to verify arbitrary files with the existing API. It may not be the nicest or sanest API but still, should be possible as-is.

DemiMarie commented 2 years ago

Thank you for ping. DNF team is looking for a replacement of GPGME for verification of signed metadat in repositories, therefore if there will be an option to extend RPM capability to also verify alternative file using keys not only from internal RPMDB keyring it would be great and everyone will benefit from such a solution or we will share the same pain. It also means that DNF would like to help with solution development and support. Of course there are some limitation - it will be difficult to accept a solution that will enlarge a minimal image.

Are you referring to the newly introduced Sequoia dependency?

j-mracek commented 2 years ago

We are opened to any solution even Sequoia, but after the last DNF community meetings it looks like that there is a price in installations requirements that will be may be to much for us.

nwalfield commented 2 years ago

@j-mracek Thanks for following up! Can you elaborate on the requirements? (Or perhaps point me to a document or issue or...)

DemiMarie commented 2 years ago

We are opened to any solution even Sequoia, but after the last DNF community meetings it looks like that there is a price in installations requirements that will be may be to much for us.

That is going to be tricky. The current implementation is deprecated and has known security problems related to key revocation. It also does not support armored signatures. I have some C++ code (not yet published) that can handle dearmoring, but there is no way I will be able to implement certificate canonicalization.

pmatilai commented 2 years ago

We are opened to any solution even Sequoia, but after the last DNF community meetings it looks like that there is a price in installations requirements that will be may be to much for us.

This seems a bit peculiar, as rpm will be using Sequoia based solution anyway. For an idea about the "cost", see https://download.copr.fedorainfracloud.org/results/decathorpe/sequoia-test-builds/fedora-36-x86_64/04419972-rust-rpm-sequoia/

The library is about 1.8M in size and the only "extra" dependency is nettle for the low-end crypto, and this replaces any other crypto dependencies in rpm (ie openssl/gcrypt). AIUI Sequoia supports openssl as an alternative too.

teythoon commented 2 years ago

AIUI Sequoia supports openssl as an alternative too.

We don't currently support OpenSSL as backend, but if there is interest in that we could add it (now that OpenSSL 3 is out).

nwalfield commented 2 years ago

To elaborate on what @teythoon said: Adding OpenSSL support shouldn't be a major undertaking. Sequoia already supports three cryptographic libraries (Nettle, Rust Crypto and Windows CNG), so the existing abstractions probably won't require any refactoring. Nevertheless, it would be good to understand why Nettle is not sufficient. AIUI, Nettle is officially supported by RHEL.

pmatilai commented 2 years ago

Doh, sorry I realize now that I mixed it with something else just recently gaining the openssl option.

The problem with nettle is that it's not openssl :laughing: I mean, for minimal container images and such people are pushing quite hard to minimize the dependencies and that includes consolidating on a single crypto library to the extent possible. The higher up in the stack you go, the less it matters because a crypto library gets lost in the other noise, but in down at the plumbing level where rpm lives, it still matters. And openssl just happens to be the most ubiquitous of those libraries, for the better or worse, and so that's what the consolidation efforts tend to gravitate to.

nwalfield commented 2 years ago

for minimal container images and such people are pushing quite hard to minimize the dependencies and that includes consolidating on a single crypto library to the extent possible.

I hadn't considered that. Thanks for taking the time to follow up.

pmatilai commented 2 years ago

Oh, and the other thing about openssl is FIPS, which AIUI nettle lacks. I may not care but it's a big deal in the enterprise world.

DemiMarie commented 2 years ago

Oh, and the other thing about openssl is FIPS, which AIUI nettle lacks. I may not care but it's a big deal in the enterprise world.

RHEL has FIPS certification for its OpenSSL library, though I think it just has OpenSSL call into the kernel crypto API (only semi-reasonable option, IMO).

j-mracek commented 2 years ago

@j-mracek Thanks for following up! Can you elaborate on the requirements? (Or perhaps point me to a document or issue or...)

I am sorry for the late answer.

DNF needs to verify RPM GPG signature and for that purpose we use RPM API. When it fails DNF imports GPG keys into RPM DB. Because it is user confirmed operation we need to provide information about a key that will be important to user and for that purpose we use gpgme to parse the key.

For verification of repositories we use gpgme and we store imported keys in certain destination for particular repository (outside of rpm DB). Even for each user the location of imported keys differs. For verification of repositories we not only want to replace gpgme but also we would like to improve user experience. We would like to use imported keys in RPM DB as a primary source of GPG keys and only when it fails we want to use keys imported for particular repository at user specific location and then when it fails we can try to import fresh keys. What is completely missing in our interface for handling repo gpg keys is a management of already imported keys.

nwalfield commented 2 years ago

Thanks for following up, that is helpful. Could you also comment on why using Sequoia would increase the cost of the installation requirements too much.

j-mracek commented 2 years ago

Thanks for following up, that is helpful. Could you also comment on why using Sequoia would increase the cost of the installation requirements too much.

I did not have a time to test the RPM with Sequoia backend but according to Panu it should be not a problem. On the other hand if we can make our stack smaller (dnf5, rpm) it would be beneficial for everyone especially in minimal containers.

wiktor-k commented 2 years ago

For the record I'm working on adding OpenSSL backend to Sequoia PGP and while the signatures (signing and verification) are already working for both RSA keys and NIST curves the entire backend is not complete yet. Notably missing are: OCB and ed25519.

I see we have a master ticket for crypto backends: https://gitlab.com/sequoia-pgp/sequoia/-/issues/333 but nothing OpenSSL specific :sweat_smile:

As for the timeline I plan on submitting a PR/MR early September for review (when I'll get all tests passing). If you want to I can notify you when it's complete and merged.

pmatilai commented 2 years ago

As for the timeline I plan on submitting a PR/MR early September for review (when I'll get all tests passing). If you want to I can notify you when it's complete and merged.

That would be appreciated.

wiktor-k commented 2 years ago

This took a liiiitle bit longer than I expected but I've submitted a patch to Sequoia to include OpenSSL backend: https://gitlab.com/sequoia-pgp/sequoia/-/merge_requests/1361

It passes all tests (and adds some more) and I'm planning to run the test suite from rpm against it but for details just follow the link above.

Have a nice day folks! :wave:

pmatilai commented 2 years ago

Just encountered https://github.com/rpm-software-management/dnf5/blob/8f46d64fb1a44b307bae5fed7ce5ba233ea0ab0e/libdnf/rpm/rpm_signature.cpp#L107

Like the old wisdom goes, perfect is the enemy of good (enough) and offering something significantly better than this is not hard, even if it may not be purrfect.

wiktor-k commented 1 year ago

For people that track the subject the OpenSSL backend has been merged and released in sequoia-openpgp crate 1.13.0: https://gitlab.com/sequoia-pgp/sequoia/-/blob/main/openpgp/NEWS#L6

Have a nice day! 👋

pmatilai commented 1 year ago

Awesome, thanks for all the work on this (to everybody involved)!

pmatilai commented 1 year ago

Okay, back to our scheduled program :laughing:

With the work to add better error reporting in #2453, it suddenly looks more appetizing to improve the upper APIs too. Right now the API is basically "here's the keyring, see if something fits". Which is sufficient for rpm's own current use, but eg dnf wants to track keys per repo, which we don't handle at all.

So it seems the lowest level public verification API should take a key instead of a keyring, and for that the keyring needs to provide an API to look up keys. We had one but it was just recently axed because it was so bad otherwise... So currently that's kinda backwards: the API that only rpm needs is public, and the ones that others need are private :facepalm:

Then on top of that, we need that package siganture verification, which also needs to take just a key, take at least vsflags for controlling operation and have error message return pointer. I need to take a closer look at https://gitlab.com/dkg/openpgp-stateless-cli/-/issues/32...

nwalfield commented 1 year ago

There should be a function to read a keyring (binary, or one or more concatenated ASCII-armor blocks) and extract all of the certificates, cf. https://github.com/rpm-software-management/rpm/issues/2487 . In Sequoia, this is done with CertParser.

nwalfield commented 1 year ago

API considerations: https://github.com/rpm-software-management/rpm/issues/189

nwalfield commented 1 year ago

A new API should return better error messages:

We'll need something that can apply to all our backends, and that something needs to have a sane means of returning error messages.

https://github.com/rpm-software-management/rpm/pull/2097#issuecomment-1319690012

https://github.com/rpm-software-management/rpm/issues/2127

nwalfield commented 1 year ago

As per https://github.com/rpm-software-management/rpm/issues/2577#issuecomment-1719074225 :

trying to shoehorn the keys into something resembling a package has been a major source of headache as long as it's been there. It's just difficult to get rid of. Ideally this would all happen in some blackbox keyring and rpm doesn't need to care.

nwalfield commented 1 year ago

It should be possible to import binary certificates, see https://github.com/rpm-software-management/rpm/issues/2689

nwalfield commented 1 year ago

Make it easy to implement rpmkeys --list-keys and rpmkeys --delete-keys.

https://github.com/rpm-software-management/rpm/issues/2404#issuecomment-1766034780

pmatilai commented 11 months ago

Another related API improvement that is hardly worth it in itself but something that could be addressed whenever https://github.com/rpm-software-management/rpm/issues/2487 is: #2513

pmatilai commented 8 months ago

I don't see this happening in 4.20, dropping from the milestone.

nwalfield commented 8 months ago

I think dropping this from the 4.20 milestone makes sense. This is a fair amount of work, and I don't currently have the time or resources to do it well.

nwalfield commented 7 months ago

As @mlschroe points out here, there should be a mechanism to update a certificate. That is, rather than importing or replacing a certificate, it should merge in certificate updates. Perhaps we should have a mechanism to reach out to public directories or a fixed URL.