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.

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

Provide a decent API for verifying package signatures #2041

Open
DemiMarie opened this issue Apr 28, 2022 · 39 comments
Open

Provide a decent API for verifying package signatures #2041

DemiMarie opened this issue Apr 28, 2022 · 39 comments
Labels
API API related crypto Signatures, keys, hashes and their verification design Complicated design issue RFE

Comments

@DemiMarie
Copy link
Contributor

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:

  • RPM_VERIFY_STRICT: request strict checking of the various headers in the package, including checks which might be incompatible with ancient broken packages.
  • RPM_VERIFY_STRONG: enforce the use of strong cryptography, even if not required by system-wide policies.
  • RPM_VERIFY_ALL: enforce all possible checks.
@pmatilai
Copy link
Member

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.

@pmatilai pmatilai added RFE API API related labels Apr 29, 2022
@DemiMarie
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

nwalfield commented Apr 29, 2022

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
Copy link
Member

pmatilai commented May 3, 2022

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

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
Copy link
Member

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
Copy link
Contributor Author

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

j-mracek commented May 10, 2022

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
Copy link
Contributor

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

@DemiMarie
Copy link
Contributor Author

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
Copy link
Member

pmatilai commented Jun 3, 2022

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

teythoon commented Jun 3, 2022

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
Copy link
Contributor

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
Copy link
Member

pmatilai commented Jun 3, 2022

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 😆 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
Copy link
Contributor

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
Copy link
Member

pmatilai commented Jun 3, 2022

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
Copy link
Contributor Author

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

j-mracek commented Jul 8, 2022

@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
Copy link
Contributor

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

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

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 😅

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
Copy link
Member

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

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

@pmatilai pmatilai added this to the 4.19.0 milestone Oct 14, 2022
@pmatilai
Copy link
Member

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

wiktor-k commented Jan 7, 2023

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
Copy link
Member

pmatilai commented Jan 9, 2023

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

@pmatilai
Copy link
Member

Okay, back to our scheduled program 😆

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 🤦

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
Copy link
Contributor

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. #2487 . In Sequoia, this is done with CertParser.

@nwalfield
Copy link
Contributor

API considerations: #189

@nwalfield
Copy link
Contributor

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.

#2097 (comment)

#2127

@pmatilai pmatilai modified the milestones: 4.19.0, 4.20.0 Aug 25, 2023
@pmatilai pmatilai added the crypto Signatures, keys, hashes and their verification label Sep 14, 2023
@nwalfield
Copy link
Contributor

As per #2577 (comment) :

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.

@pmatilai pmatilai removed their assignment Oct 3, 2023
@nwalfield
Copy link
Contributor

It should be possible to import binary certificates, see #2689

@nwalfield
Copy link
Contributor

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

#2404 (comment)

@pmatilai
Copy link
Member

Another related API improvement that is hardly worth it in itself but something that could be addressed whenever #2487 is: #2513

@pmatilai
Copy link
Member

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

@nwalfield
Copy link
Contributor

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
Copy link
Contributor

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.

@pmatilai pmatilai added the design Complicated design issue label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API related crypto Signatures, keys, hashes and their verification design Complicated design issue RFE
Projects
Status: Todo
Development

No branches or pull requests

6 participants