[Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)

[Neal H. Walfield]

@nwalfield commented on this pull request.

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"?


> @@ -14,6 +14,16 @@
 extern "C" {
 #endif
 
+/** \ingroup rpmkeyring
+  * Operation mode definitions for rpmKeyringModify
+  */
+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.

> @@ -101,6 +111,15 @@ char * rpmPubkeyBase64(rpmPubkey key);
  */
 pgpDigParams rpmPubkeyPgpDigParams(rpmPubkey key);
 
+/** \ingroup rpmkeyring
+ * Modify the keys in the keyring
+ * @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.

> @@ -229,6 +229,28 @@ char * rpmPubkeyBase64(rpmPubkey key)
     return enc;
 }
 
+rpmRC rpmPubkeyMerge(rpmPubkey oldkey, rpmPubkey newkey, rpmPubkey *mergedkeyp)
+{
+    rpmPubkey mergedkey = NULL;
+    uint8_t *mergedpkt = NULL;
+    size_t mergedpktlen = 0;
+    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.

> @@ -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
+ * @param pkts1len	length of the buffer with first certificates
+ * @param pkts2		OpenPGP pointer to a buffer of second certificates
+ * @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`.

> @@ -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
+ * @param pkts1len	length of the buffer with first certificates
+ * @param pkts2		OpenPGP pointer to a buffer of second certificates
+ * @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)
+ * @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?

> @@ -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
+ * @param pkts1len	length of the buffer with first certificates
+ * @param pkts2		OpenPGP pointer to a buffer of second certificates
+ * @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)
+ * @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 `rpmKeyring`s.  Is there a reason we can't use `rpmKeyring`s here?

> @@ -229,6 +229,28 @@ char * rpmPubkeyBase64(rpmPubkey key)
     return enc;
 }
 
+rpmRC rpmPubkeyMerge(rpmPubkey oldkey, rpmPubkey newkey, rpmPubkey *mergedkeyp)
+{
+    rpmPubkey mergedkey = NULL;
+    uint8_t *mergedpkt = NULL;
+    size_t mergedpktlen = 0;
+    rpmRC rc;
+
+    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.

>      if (fd) {
 	size_t keylen = strlen(keyval);
 	if (Fwrite(keyval, 1, keylen, fd) == keylen)
 	    rc = RPMRC_OK;
 	Fclose(fd);
     }
+    if (!rc && tmppath && rename(tmppath, path) != 0)

Can we be sure that `tmppath` and `path` are on the same file system?  (If not, `rename` will fail with `EXDEV`.)

> @@ -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).

>  
     if (rc) {
 	rpmlog(RPMLOG_ERR, _("failed to import key: %s: %s\n"),
-		path, strerror(errno));
+		tmppath ? tmppath : path, strerror(errno));
+	if (tmppath)
+	    unlink(tmppath);
+    }
+    
+    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.)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3083#pullrequestreview-2046381259
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/3083/review/2046381259 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20240522/313dacc8/attachment.html>