[Rpm-maint] [rpm-software-management/rpm] rpmKeyring: Support keys with the same key ID (PR #3398)

[Panu Matilainen]

@pmatilai requested changes on this pull request.



> @@ -153,8 +158,14 @@ rpmPubkey rpmKeyringLookupKey(rpmKeyring keyring, rpmPubkey key)
     if (keyring == NULL || key == NULL)
 	return NULL;
     rdlock lock(keyring->mutex);
-    auto item = keyring->keys.find(key->keyid);
-    rpmPubkey rkey = item == keyring->keys.end() ? NULL : rpmPubkeyLink(item->second);
+
+    auto range = keyring->keys.equal_range(key->keyid);
+    auto item = range.first;
+    for (; item != range.second; item++) {

When at all possible, keep the loop variable inside the for. It should also be a reference, this is now making copies of the keys as it loops, and favor pre-increment. And then the whole thing becomes something like 

```
for (auto const & item = range.first; item != range.second; ++item) {
    if (item->second->fp == key->fp)
        return rpmPubkeyLink(item->second);
}
return NULL;
```

Alternatively, leave the rkey declaration before the loop, just initialized to NULL and break out as needed, that's kinda matter of style.

>  	rdlock lock(keyring->mutex);
 	auto keyid = key2str(pgpDigParamsSignID(sig));
-	auto item = keyring->keys.find(keyid);
-	if (item != keyring->keys.end()) {
+
+	/* use first verifying key or last one with matching keyid */
+	auto range = keyring->keys.equal_range(keyid);
+	auto item = range.first;
+	for (; item != range.second; item++) {

Same as above, make item a reference inside the for-loop, pre-increment.

>  	    /* Do the parameters match the signature? */
-	    if ((pgpDigParamsAlgo(sig, PGPVAL_PUBKEYALGO)
-                 != pgpDigParamsAlgo(pub, PGPVAL_PUBKEYALGO)) ||
-                memcmp(pgpDigParamsSignID(sig), pgpDigParamsSignID(pub),
-                       PGP_KEYID_LEN)) {
+	    if (pgpDigParamsAlgo(sig, PGPVAL_PUBKEYALGO)
+		!= pgpDigParamsAlgo(key->pgpkey, PGPVAL_PUBKEYALGO)) {
 		key = NULL;

The condition needs to be on a different indentation level from the code block. That part was wrong to begin with but this is a chance to fix it since we're touching that code. Typical solutions:
- move the test to a helper variable or function to make it shorter
- push the second line of condition indentation deeper
- put the opening { on the next line as per "the other" coding style - this is the one special case where is totally okay because it actually makes it quite readable

>  
-rpmRC rpmKeyringVerifySig2(rpmKeyring keyring, pgpDigParams sig, DIGEST_CTX ctx, rpmPubkey * keyptr)
-{
-    rpmRC rc = RPMRC_FAIL;
-
-    if (sig && ctx) {
-	pgpDigParams pgpkey = NULL;
-	rpmPubkey key = findbySig(keyring, sig);
-
-	if (key)
-	    pgpkey = key->pgpkey;
+	    if (pgpVerifySignature(pgpkey, sig, ctx) == RPMRC_OK)

This doesn't seem right, we're now verifying the same signature twice. It's expensive enough that we don't want to be doing that.

>  	}
-	if (keyptr) {
-	    *keyptr = pubkeyPrimarykey(key);
+
+	/* keys but no one verifies: show errors from all */
+	if (range.first != range.second && !key) {
+	    for (item = range.first; item != range.second; item++) {
+		key = item->second;
+		rc = pubkeyVerifySig(key, sig, ctx);

Don't reverify, just collect any errors on the first run and then log them all if  the result is a failure.

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

Message ID: <rpm-software-management/rpm/pull/3398/review/2384805905 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20241023/8a5a7c48/attachment.html>