@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/2046381...@github.com>_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
