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

OpenPGP: Function pgpParsePkts supports only "PGP PUBLIC KEY BLOCK" block, "PGP SIGNATURE" is needed #2512

Closed
jrohel opened this issue May 18, 2023 · 9 comments
Labels
crypto Signatures, keys, hashes and their verification

Comments

@jrohel
Copy link

jrohel commented May 18, 2023

I am preparing a new backend for OpenPGP support in librepo rpm-software-management/dnf5#438 . Instead of GpgMe, it uses the librpm library API. The goal is to get rid of the dependency on GpgMe.

I need support for loading ASCII armored PGP signatures. I found it works with "rpm-sequoia" backend. The problem is with internal librpm implementation.

Is there a plan to add support for ASCII armored PGP signatures to librpm's internal implementation? Or will an external implementation ("rpm-sequouia" backend) be required in future versions (e.g. next Fedora)?

@DemiMarie
Copy link
Contributor

Is there a plan to add support for ASCII armored PGP signatures to librpm's internal implementation?

Highly unlikely. The internal implementation is only for backwards compatibility with environments that for some reason are not willing to use the Sequoia implementation. It has known bugs that almost certainly won’t be fixed. To give just one example, revocation handling is not implemented at all.

Or will an external implementation ("rpm-sequouia" backend) be required in future versions (e.g. next Fedora)?

It’s already the default in Fedora 38.

@mlschroe
Copy link
Contributor

I'd argue that either rpm-sequoia or the internal implementation should be fixed. I'm not sure which is correct.

@jrohel
Copy link
Author

jrohel commented May 19, 2023

@mlschroe
In this case, sequoia is fine. But the internal implementation is incomplete. But in my opinion, there was no pressure to complete it. We did not use this API. We used GpgMe.

Details:
The pgpReadPkts function returns the pgpArmor enum, which since its creation in 2001, contains an entry for the signature PGPARMOR_SIGNATURE (and other entries). And the function is described: "Parse armored OpenPGP packets from a file." And it returns "type of armor found".
In fact, it only parses one type PGPARMOR_PUBKEY.

/** \ingroup rpmpgp
 * Parse armored OpenPGP packets from a file.
 * @param fn		file name
 * @param[out] pkt	dearmored OpenPGP packet(s) (malloced)
 * @param[out] pktlen	dearmored OpenPGP packet(s) length in bytes
 * @return		type of armor found
 */
pgpArmor pgpReadPkts(const char * fn, uint8_t ** pkt, size_t * pktlen);
    PGPARMOR_MESSAGE		=  1, /*!< MESSAGE */
    PGPARMOR_PUBKEY		=  2, /*!< PUBLIC KEY BLOCK */
    PGPARMOR_SIGNATURE		=  3, /*!< SIGNATURE */
    PGPARMOR_SIGNED_MESSAGE	=  4, /*!< SIGNED MESSAGE */
    PGPARMOR_FILE		=  5, /*!< ARMORED FILE */
    PGPARMOR_PRIVKEY		=  6, /*!< PRIVATE KEY BLOCK */
    PGPARMOR_SECKEY		=  7  /*!< SECRET KEY BLOCK */

@mlschroe
Copy link
Contributor

I dunno about that "fine" and "incomplete". You're asking to remove an extra check in the internal pgp parser.
So maybe the sequoia code is incomplete.

Basically the API is missing a type argument to tell the parser if it should test for a certain armor.

@nwalfield
Copy link
Contributor

@jrohel: Why does librepo need to parse signature files? Can you point me to the code that relies on this behavior, please.

@mlschroe: I agree. rpm-sequoia probably should reject these signatures.

@jrohel
Copy link
Author

jrohel commented May 21, 2023

@mlschroe
Sorry, I'm not a great English speaker, but I assume that when the function is described as Parse armored OpenPGP packets from a file. and returns type of armor found, it will return PGPARMOR_SIGNATURE after finding the signature. Especially when that value is part of the returned enum.
Or something like "NOT_IMPLEMENTED" is also understandable. But PGPARMOR_NONE? I deduce from the description of the function that this is not the expected result for the signature.

@jrohel
Copy link
Author

jrohel commented May 21, 2023

@nwalfield

Why does librepo need to parse signature files?

Librepo is used to download data from rpm repositories (metadata, packages, ...). Repository metadata can be signed with an OpenPGP signature. And librepo can verify them.

Sample on an existing repository:

[google-chrome]
name=google-chrome
baseurl=https://dl.google.com/linux/chrome/rpm/stable/x86_64
skip_if_unavailable=True
gpgcheck=1
gpgkey=https://dl.google.com/linux/linux_signing_key.pub
enabled=1
repo_gpgcheck=1

gpgkey is a URL to a file containing multiple blocks of ASCII armored public keys (there can be multiple URLs.)
The repository contains a "repomd.xml" file and a "repomd.xml.asc" file. The "repomd.xml.asc" file is an ASCII armored OpenPGP signature that librepo needs to load in order to verify the "repomd.xml" file.

Librepo uses GpgMe. We now have a high priority to remove the dependency on GpgMe. That's why I created a new implementation in the librepo that uses the librpm API instead of GpgMe. Now it works (with some problems that I described in issues, but it works).
If the pgpParsePkts function does not support ASCII armored signature parsing, how do I load it in the librepo? New better function? Okay, but we need it quickly.

@jrohel
Copy link
Author

jrohel commented May 21, 2023

@nwalfield

Can you point me to the code that relies on this behavior, please.

Here is the PR rpm-software-management/librepo#275 . It contains commit that moves the original implementation of OpenPGP using GpgMe into "gpg_gpgme.c" file and creates a new based on librpm API in the "gpg_rpm.c" file.
There is a function check_signature which internally calls pgpParsePkts to parse the ASCII armored OpenPGP signature.

I created the code based on the description in the "rpmgpg.h" header file and a bit of librpm reverse engineering. It's not very nice, but somehow it works. The code also implements a keyring.

@pmatilai
Copy link
Member

Considering that the internal parser is on its way out, and changing rpm-sequoia to reject data that the return values suggest the function supports, breaking librepo in the process... I think the only reasonable thing is to just leave it all as it is. There are and will be significant differences in the backends as long as the internal one is there, so it seems to me the sooner we get rid of it the better it is for everybody, ultimately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Signatures, keys, hashes and their verification
Projects
None yet
Development

No branches or pull requests

5 participants