Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@pmatilai commented on this pull request.
> @@ -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(>lock);
+pthread_rwlock_rdlock(>lock);
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1611406952
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@pmatilai commented on this pull request. > @@ -509,6 +509,19 @@ int pgpSignatureType(pgpDigParams sig); */ char *pgpIdentItem(pgpDigParams digp); +/** \ingroup rpmpgp + * Merge the PGP packets of two public keys + * @param pkts1OpenPGP pointer to a buffer of first certificates + * @param pkts1len length of the buffer with first certificates + * @param pkts2OpenPGP 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 flagsmerge flags (currently not implemented) 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). -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1611400268 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@nwalfieldthanks for the review! -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2124719422 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request.
>
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 */
see above.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609890493
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request. > @@ -509,6 +509,19 @@ int pgpSignatureType(pgpDigParams sig); */ char *pgpIdentItem(pgpDigParams digp); +/** \ingroup rpmpgp + * Merge the PGP packets of two public keys + * @param pkts1OpenPGP pointer to a buffer of first certificates That's a copy/paste error ;-) -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609889800 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request.
> 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)
It's in the same directory (tmppath = path + ".new")
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609888398
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request. > @@ -509,6 +509,19 @@ int pgpSignatureType(pgpDigParams sig); */ char *pgpIdentItem(pgpDigParams digp); +/** \ingroup rpmpgp + * Merge the PGP packets of two public keys + * @param pkts1OpenPGP pointer to a buffer of first certificates + * @param pkts1len length of the buffer with first certificates + * @param pkts2OpenPGP 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 flagsmerge 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); All the other functions use buffers as well (e.g. pgpParsePkts, pgpPubKeyCertLen, ...) -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609879838 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request. > @@ -509,6 +509,19 @@ int pgpSignatureType(pgpDigParams sig); */ char *pgpIdentItem(pgpDigParams digp); +/** \ingroup rpmpgp + * Merge the PGP packets of two public keys + * @param pkts1OpenPGP pointer to a buffer of first certificates + * @param pkts1len length of the buffer with first certificates + * @param pkts2OpenPGP 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 flagsmerge flags (currently not implemented) + * @return RPMRC_OK on success 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609877474 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request. > @@ -509,6 +509,19 @@ int pgpSignatureType(pgpDigParams sig); */ char *pgpIdentItem(pgpDigParams digp); +/** \ingroup rpmpgp + * Merge the PGP packets of two public keys + * @param pkts1OpenPGP pointer to a buffer of first certificates + * @param pkts1len length of the buffer with first certificates + * @param pkts2OpenPGP 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 flagsmerge flags (currently not implemented) 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609873045 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request.
> @@ -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(>lock);
+pthread_rwlock_rdlock(>lock);
I don't know. I'm not even sure that we need to lock them but found it more
safe. Panu?
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609869928
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@mlschroe commented on this pull request.
> @@ -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
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3083#discussion_r1609868304
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
> 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2124591840 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@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(>lock);
+pthread_rwlock_rdlock(>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 pkts1OpenPGP pointer to a buffer of first
certificates
+ * @param pkts1len length of the buffer with first certificates
+ * @param pkts2OpenPGP 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 flagsmerge 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 pkts1OpenPGP pointer to a buffer of first
certificates
+ * @param pkts1len length of the buffer with first certificates
+ * @param pkts2OpenPGP 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 flagsmerge 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 pkts1OpenPGP pointer to a buffer of first
certificates
+ * @param pkts1len length of the buffer with first certificates
+ * @param pkts2OpenPGP 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 flagsmerge 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
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2122328519 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
@pmatilai approved this pull request. Looks fine to me! @nwalfield, if you can afford a few cycles to look at this before I hit merge, I'd appreciate... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#pullrequestreview-2067603707 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
maybe "pgp" would be a working name for the backend... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2117450107 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
I started fixing some problems I found in the code and then got carried away... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2115221208 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
> 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 :sweat_smile: -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2111729619 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
> 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! -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2104486277 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
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 :+1: @nwalfield , thoughts from rpm-sequoia POV (or otherwise)? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083#issuecomment-2100369992 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Implement merging of new key material when importing pubkeys (PR #3083)
This currently only makes a difference if the legacy backend is used. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/3083 -- Commit Summary -- * Fix error handling in rpmtsImportPubkey * Make subkeys available right away when a pubkey is imported * Split subkey initialization into its own function * Add rpmKeyringModify to change the keys of a keyring * Add a method to lookup a key from the keyring * Support pubkey merging in the keyring code * Merge keys and update the database in rpmtsImportPubkey() -- File Changes -- M include/rpm/rpmkeyring.h (35) M include/rpm/rpmpgp.h (13) M lib/rpmts.c (103) M rpmio/rpmkeyring.c (78) M rpmio/rpmpgp_dummy.c (5) M rpmio/rpmpgp_sequoia.c (8) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/3083.patch https://github.com/rpm-software-management/rpm/pull/3083.diff -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3083 You are receiving this because you are subscribed to this thread. Message ID: rpm-software-management/rpm/pull/3...@github.com ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
