Implement merging of new key material when importing pubkeys #3083

mlschroe · 2024-05-08T11:20:15Z

This currently only makes a difference if the "legacy" backend is used.

The call to pgpPubKeyLint() modified the rc code, which the following code assumed to be RPMRC_FAIL. Also, the return code of the database operation was always overwritten with RPMRC_OK.

This is useful for API users that do not re-read the keys from the database.

No functual changes.

pmatilai · 2024-05-08T11:32:31Z

Oh, nice. Didn't look at details yet but the functionality is pretty desperately needed indeed.
The current behavior of just bailing out if main keyid is already there predates the subkey support by many years and was only ever intended as a stop-gap behavior until something better gets done. Well, here we are, finally 👍

@nwalfield , thoughts from rpm-sequoia POV (or otherwise)?

nwalfield · 2024-05-10T12:01:06Z

Oh, nice. Didn't look at details yet but the functionality is pretty desperately needed indeed. The current behavior of just bailing out if main keyid is already there predates the subkey support by many years and was only ever intended as a stop-gap behavior until something better gets done. Well, here we are, finally 👍

@nwalfield , thoughts from rpm-sequoia POV (or otherwise)?

I'll take a look the start of next week. From a very high-level perspective: yes, we want to merge certificates. So, thanks a lot for working on this @mlschroe!

pmatilai · 2024-05-15T07:00:01Z

the "legacy" backend

@mlschroe , I didn't quite expect you to start so actively hacking on it, more like terminal care and hence the name. If you intend to continue developing it, I'm okay with renaming it to something else than "legacy". Only it can't be "internal" anymore because it's not 😅

mlschroe · 2024-05-16T13:16:23Z

I started fixing some problems I found in the code and then got carried away...

adrianschroeter · 2024-05-17T12:04:53Z

maybe "pgp" would be a working name for the backend...

pmatilai

Looks fine to me!

@nwalfield, if you can afford a few cycles to look at this before I hit merge, I'd appreciate...

mlschroe · 2024-05-21T10:42:05Z

About that "legacy" name: how about changing it to "traditional"? It doesn't sound so negative and also suggests that the code is somewhat not state of the art.

nwalfield

Thanks a lot for working on this. The most important question I have regards the proposed semantics. You have "Add", "Replace", and "Delete." I don't understand the point of "Replace." Do you have a use case for when you want to replace and not merge? Why not "AddOrMerge" and "Delete"?

nwalfield · 2024-05-08T18:32:05Z

include/rpm/rpmkeyring.h

+typedef enum rpmKeyringModifyMode_e {
+    RPMKEYRING_ADD	= 1,
+    RPMKEYRING_REPLACE	= 2,
+    RPMKEYRING_DELETE	= 3


Could you please explain the semantics of these different operations. My feeling is that there are only two relevant operations: ADD_OR_MERGE and DELETE. Does your ADD fail if the certificate is already present? Does REPLACE do a merge or not (looking at the code, it doesn't appear to)? If it doesn't, why not? (What's the use case?) Doesn't not merging mean that you potentially lose old binding signatures? That seems problematic to me.

This is about if you do the merge in the keyring code or in the import code. I played with both options and then settled in doing the merge call in the import code and then replacing the old pubkey with the merged pubkey.

It's up to the merge code if it wants to keep all binding signatures. The rpmpgp_legacy merge code does not throw away any pgp package.

nwalfield · 2024-05-08T18:33:03Z

include/rpm/rpmkeyring.h

+ * @param key		Pubkey
+ * @param key		pubkey handle
+ * @param mode          mode of operation
+ * @return		0 on success, -1 on error, 1 if key already present (add) or not found (delete)


Can you please make this comment a bit more verbose, please. I get the idea, but the semantics aren't entirely clear to me.

nwalfield · 2024-05-08T18:42:19Z

rpmio/rpmkeyring.c

+    rpmRC rc;
+
+    pthread_rwlock_rdlock(&oldkey->lock);
+    pthread_rwlock_rdlock(&newkey->lock);


Do these locks have a locking order? If not, we should define one.

I don't know. I'm not even sure that we need to lock them but found it more safe. Panu?

I don't think we have a defined locking order for anything in rpm. The locks on they keys are probably a bit hokey to begin with because librpm and threads is playing with fire anyway. But, it'd seem unsafe to drop since we have them elsewhere.

A simple locking order would be to lock according to the memory address. But, that is probably out of scope for this PR.

nwalfield · 2024-05-08T18:44:21Z

include/rpm/rpmpgp.h

+ * @param pkts2len	length of the buffer with second certificates
+ * @param pktsm[out]	merged certificates (malloced)
+ * @param pktsmlen[out]	length of merged certificates
+ * @param flags		merge flags (currently not implemented)


This should probably say: must be 0.

We can also drop the flags. I added them because we could use them in the future to change the merge operation. E.g. have a flag to ignore all non-self signatures or similar.

I'd rather have a flags arg we never end up using than not having it when we need it. Rpm is traditionally terribly sloppy with these, but indeed it'd be good to state the only legit value now is 0 so people don't end up passing random stuff that hurts us later (been there).

nwalfield · 2024-05-08T18:46:15Z

include/rpm/rpmpgp.h

+ * @param pktsm[out]	merged certificates (malloced)
+ * @param pktsmlen[out]	length of merged certificates
+ * @param flags		merge flags (currently not implemented)
+ * @return 		RPMRC_OK on success 


Could you please clarify the semantics. What exactly is merging? Does it mean the union of the keys or also the union of self-signatures? (I hope it means the latter.) What if the two certificates are not the same? What if a buffer contains multiple certificates? What if a certificate is invalid?

The pgp backend MUST return an error if a buffer contains multiple certificates or is invalid.

How the merging is done is up to the backend implementer. It SHOULD build the union of all pgp packets in some sane order and remove all duplicates.

nwalfield · 2024-05-08T19:12:47Z

include/rpm/rpmpgp.h

+ * @param flags		merge flags (currently not implemented)
+ * @return 		RPMRC_OK on success 
+ */
+rpmRC pgpPubkeyMerge(const uint8_t *pkts1, size_t pkts1len, const uint8_t *pkts2, size_t pkts2len, uint8_t **pktsm, size_t *pktsmlen, int flags);


It's a bit unfortunate that this interface works with buffers and not rpmKeyrings. Is there a reason we can't use rpmKeyrings here?

All the other functions use buffers as well (e.g. pgpParsePkts, pgpPubKeyCertLen, ...)

nwalfield · 2024-05-08T19:16:03Z

rpmio/rpmkeyring.c

+    pthread_rwlock_rdlock(&oldkey->lock);
+    pthread_rwlock_rdlock(&newkey->lock);
+    rc = pgpPubkeyMerge(oldkey->pkt.data(), oldkey->pkt.size(), newkey->pkt.data(), newkey->pkt.size(), &mergedpkt, &mergedpktlen, 0);
+    if (rc == RPMRC_OK && (mergedpktlen != oldkey->pkt.size() || memcmp(mergedpkt, oldkey->pkt.data(), mergedpktlen) != 0)) {


This is unfortunate. It assumes that there is a canonical form. Sequoia will first canonicalize the certificates, and then merge them. It's able to detect that nothing new was added, but may not emit the same bytes. I'd rather have pgpPubkeyMerge return "unchanged" in this case.

Seems I forgot to press the "Comment" button after I typed my reply...

There was no straightforward option with the RPMRC codes, so I went with the memcmp as a way to detect that nothing was changed. We can certainly hijack a RPMRC code or use a different means

But note that the rpm-sequoia code can also just duplicate the first certificate if the sequoia library returns an "unchanged" status...

lib/rpmts.c

nwalfield · 2024-05-22T08:55:50Z

include/rpm/rpmpgp.h

@@ -509,6 +509,19 @@ int pgpSignatureType(pgpDigParams sig);
 */
 char *pgpIdentItem(pgpDigParams digp);

+/** \ingroup rpmpgp
+ * Merge the PGP packets of two public keys
+ * @param pkts1		OpenPGP pointer to a buffer of first certificates


Do you really mean this function should handle multiple certificates, i.e., an OpenPGP keyring? If so, then we only need a single buffer. Otherwise, s/certificates/certificate/ (likewise below).

That's a copy/paste error ;-)

nwalfield · 2024-05-22T11:36:53Z

lib/rpmts.c

+    }
+
+    if (!rc && replace) {
+	/* find and delete the old pubkey entry */


What's the use case for these semantics? (Deleting the old version of a certificate and replacing it with a new version.)

nwalfield · 2024-05-22T11:45:03Z

Looks fine to me!

@nwalfield, if you can afford a few cycles to look at this before I hit merge, I'd appreciate...

Sorry for the delay. I'm in grant writing mode as our funding from STF ends at the end of this year, and so far we haven't gotten any other sponsors---public, commercial, or otherwise.

mlschroe · 2024-05-22T12:50:48Z

@nwalfieldthanks for the review!

pmatilai · 2024-06-11T07:18:43Z

Looks to me the issues are mostly cosmetical, like a mention of copy-paste error. @mlschroe , can you fixup the review remarks so we can merge, or are you planning further work on this?

mlschroe · 2024-06-11T11:12:54Z

I'll do the requested changes to the comments and force push. After that it should be in a state that can be merged.

This allows us to replace or delete keys from the keyring.

Use this method in rpmtsImportPubkey() to check if we already have that key. This is the place where we will implement key merging in the next commits.

The new rpmPubkeyMerge function will merge the certificate material of two pubkeys describing the same key. This is currently only implemented in the "legcay" backend.

mlschroe · 2024-06-13T14:43:23Z

Ok, I think this is now ready to be merged.

pmatilai · 2024-06-14T05:54:39Z

Ack, lets get it merged then, been open for comments long enough for sure.
And it's not as if we couldn't change things once merged if need be.

pmatilai · 2024-06-14T05:55:36Z

Thanks a lot for working on this @mlschroe and @nwalfield for the review! This topic has become a bit of a hot potato in recent months.

pmatilai · 2024-06-24T07:19:22Z

Just FWIW, it'd be nice to get this into 4.20 too, but the backport is more involved just a plain cherry-pick so I figured it's best I don't mess with it just before heading off to vacation. But, patches would be cheerfully accepted 😇

nwalfield · 2024-07-09T09:57:17Z

FYI, I've implemented this in rpm-sequoia. The change is part of v1.7.0.

FrostyX · 2024-11-01T16:57:04Z

Thank you all for working on this.
Many Copr users encountered this issue, so the fix is greatly appreciated.

As I understand the discussion, the feature should already be released and available?

I am testing it in a F41 container:

$ rpm -q rpm
rpm-4.20.0-1.fc41.x86_64
$ rpm -q rpm-sequoia
rpm-sequoia-1.7.0-2.fc41.x86_64

And this is my reproducer:

$ dnf install faketime diffutils

# Download some keys
$ curl https://raw.githubusercontent.com/xsuchy/distribution-gpg-keys/64392d70990254bb10876d68b451c0242dde4f95/keys/copr/copr-agriffis-neovim-nightly.gpg > old.gpg
$ curl https://raw.githubusercontent.com/xsuchy/distribution-gpg-keys/83ac2d8fd74a595687b0ecbb04081864cf59da01/keys/copr/copr-agriffis-neovim-nightly.gpg > new.gpg

# Pretend we imported an old key 5 years ago
$ faketime '-5years' rpm --import old.gpg

# See information about the key
$ rpm -q gpg-pubkey
gpg-pubkey-e99d6ad1-64d2612c
gpg-pubkey-453d6413-5c7aa779

$ rpm -qi gpg-pubkey-453d6413-5c7aa779 > old

# Import a new key
$ rpm --import new.gpg

# None of the information changed
$ rpm -q gpg-pubkey
gpg-pubkey-e99d6ad1-64d2612c
gpg-pubkey-453d6413-5c7aa779

$ rpm -qi gpg-pubkey-453d6413-5c7aa779 > new
$ diff old new
$

So it doesn't seem to work. Am I testing the feature correctly?

pmatilai · 2024-11-04T07:19:37Z

The rpm-side feature only exists in git master, not any released rpm version.

mlschroe added 3 commits May 8, 2024 11:12

Fix error handling in rpmtsImportPubkey

4621abc

The call to pgpPubKeyLint() modified the rc code, which the following code assumed to be RPMRC_FAIL. Also, the return code of the database operation was always overwritten with RPMRC_OK.

Make subkeys available right away when a pubkey is imported

0872648

This is useful for API users that do not re-read the keys from the database.

Split subkey initialization into its own function

5926604

No functual changes.

pmatilai mentioned this pull request May 13, 2024

Refresh PGP keys, e.g. when prolonging an expiration time rpm-software-management/dnf5#1192

Open

jan-kolarik mentioned this pull request May 14, 2024

expired-gpg-keys: new plugin to detect expired GPG keys rpm-software-management/dnf-plugins-core#533

Merged

pmatilai approved these changes May 21, 2024

View reviewed changes

nwalfield reviewed May 22, 2024

View reviewed changes

pmatilai mentioned this pull request Jun 6, 2024

urpmi --no-verify is broken by rpm checking on its own with rpm-sequoia #3142

Closed

mlschroe added 4 commits June 13, 2024 16:06

Add rpmKeyringModify to change the keys of a keyring

367f8ed

This allows us to replace or delete keys from the keyring.

Add a method to lookup a key from the keyring

da3a950

Use this method in rpmtsImportPubkey() to check if we already have that key. This is the place where we will implement key merging in the next commits.

Support pubkey merging in the keyring code

1b3dc11

The new rpmPubkeyMerge function will merge the certificate material of two pubkeys describing the same key. This is currently only implemented in the "legcay" backend.

Merge keys and update the database in rpmtsImportPubkey()

e3f971f

mlschroe force-pushed the master branch from 028d84b to e3f971f Compare June 13, 2024 14:19

pmatilai merged commit edbcb45 into rpm-software-management:master Jun 14, 2024
1 check passed

This was referenced Jun 17, 2024

Fix doc strings #3168

Merged

rpm --import does not replace old keys with new keys #2577

Closed

FrostyX mentioned this pull request Aug 7, 2024

Prolonged GPG keys are not updated on the system fedora-copr/copr#2894

Open

dmnks mentioned this pull request Aug 29, 2024

RPM 4.20.0 RC1 #3262

Merged

primeos-work mentioned this pull request Nov 21, 2024

Better OpenPGP keys handling for signing the repository packages kubernetes/release#3840

Open

Implement merging of new key material when importing pubkeys #3083

Implement merging of new key material when importing pubkeys #3083

Conversation

mlschroe commented May 8, 2024

pmatilai commented May 8, 2024

nwalfield commented May 10, 2024

pmatilai commented May 15, 2024

mlschroe commented May 16, 2024

adrianschroeter commented May 17, 2024

pmatilai left a comment

Choose a reason for hiding this comment

mlschroe commented May 21, 2024

nwalfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwalfield commented May 22, 2024

mlschroe commented May 22, 2024

pmatilai commented Jun 11, 2024

mlschroe commented Jun 11, 2024

mlschroe commented Jun 13, 2024

pmatilai commented Jun 14, 2024

pmatilai commented Jun 14, 2024

pmatilai commented Jun 24, 2024 • edited Loading

nwalfield commented Jul 9, 2024

FrostyX commented Nov 1, 2024

pmatilai commented Nov 4, 2024

pmatilai May 23, 2024 •

edited

Loading

pmatilai commented Jun 24, 2024 •

edited

Loading