rpmKeyring: Support keys with the same key ID

[ffesti]

Loop over the candidates during signature verification and use the one verifying it - iff any. Otherwise use last key with matching key ID (basically a random one).

Resolves: #3334

[ffesti]

For now the test case only has two keys with matching key IDs. Will add tests with 3 or 4 keys with the same id at some later point.

[pmatilai]

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.

[pmatilai]

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

[pmatilai]

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.

[pmatilai]

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

[ffesti]

rpmKeyringModify still has auto item = range.first; outside the loop. But this is going to change for #3350 anyway.

[pmatilai]

Being outside loop is not a crime, it's just something to generally avoid if possible.

On that note, ignore my earlier mumble about references and copies (as you did) - the thing you get from equal_range() is an iterator, not the item itself. "item" is not the best of names for it in all these cases. "it" is the c++ iterator counterpart for "int i" idiom.

rpmKeyringVerifySig2() is unnecessarily convoluted though. Just push all the lints into the vector and on non-ok final result log it all in a loop, and use a c++ string so you don't need to manually clear it at the end. The no-key sanity check should only ever occur if we didn't find a key at all, not on any failure, and should use the same exact return code handling as the keyed version - you'll want a small helper function to handle the lints collection and all.

[pmatilai]

Oh and here we get to the wonders of C++ diversity 😆 - this syntax makes copies, so except for trivialities like integers you always want to use a reference in this construct, called "range based for loop". And, whenever possible, use a const reference at that.
So: for (auto const & item) (and here "item" is appropriate because it is a reference to the item, not an iterator)

[ffesti]

OK, simplified the code. Just print out the right messages. I'd guess there shouldn't be a message for RPMRC_OK but the code for now does not make that assumption.

[pmatilai]

Okay, looks better, but there are a couple of further improvements to be made. This being security sensitive code, you'll want the logic to be as lean and mean and obvious as you possibly can.

There are two entirely separate cases here:

• no key, which can only return FAIL/NOKEY
• one or more matches, which can only return FAIL/OK
  You'll want those in a top-level if-else so there's no doubt whatsoever: only one of them can execute, and keyptr can only be set by the latter. Which means "key" can and should move to a more local scope in the second. And for the no key case, it'd be a good idea to add an actual assert to make it 200% clear: it can never ever return OK.

Add that helper function around the pgpVerifySignature2() and lints collection as I suggested in the previous comment, it's not that it's a large amount of code but to prove a point: even the simple three lines differ here, the first one using a gcc-extension syntax. Plus it'll make the business logic stand out more by isolating the C strings management out of sight entirely.

The log output logic can probably simplified a lot by realizing that if we get an OK, we can just wipe out any previous results away: with the above logic impelemented, they can only be from non-matching keys and uninteresting if we found a match. So just do results.clear() if you got OK and then you can simply log any non-empty output.

[pmatilai]

item.second.empty() would be the more appropriate test for an empty C++ string. Not that it affects the results here.

[pmatilai]

This will be moot once you add the wrapper around pgpVerifySignature2() which makes lints local, but this is another example of a variable in the wrong scope and in this case, dangerous: lints can be free'd and then accessed again later, but the pointer is not NULL'ed which is akin to leaving a booby trap in the code. Does pgpVerifySignature2() always assign to the lints pointer when passed? Maybe it does, but if not then we can end up accessing freed memory. You don't want to rely on such a thing.

[pmatilai]

And a scope-issue here too - the results vector is only needed inside the main if, so that's where it belongs. And key inside the only code that can actually end with it being set.

[pmatilai]

Variables being in the most local scope is something that really belongs to self-review part, FWIW.

[pmatilai]

Just a side-remark as I think clearing the results on OK is a better solution here, but such code can usually be made far more readable by giving the complex condition a human understandable name. Eg this might be something like

bool relevant_result = ((rc == RPMRC_OK) == (item.first == RPMRC_OK));
if (relevant_result && !item.second.empty)
    rpmlog()...

[ffesti]

There are two entirely separate cases here:

* no key, which can only return FAIL/NOKEY

* one or more matches, which can only return FAIL/OK
  You'll want those in a top-level if-else so there's no doubt whatsoever: only one of them can execute, and keyptr can only be set by the latter. Which means "key" can and should move to a more local scope in the second. And for the no key case, it'd be a good idea to add an actual assert to make it 200% clear: it can never ever return OK.

This is wrong IMHO. There may be keys that get dropped in the loop because we can figure out they do not match. If we can get the fingerprint of some signatures at some point we might be able to filter out non-matching keys even more. In these cases we still need to run the NOKEY part if all keys got dropped.

Also even in the NOKEY part we want to write NULL to the keyptr so callers can rely on their key variable getting updated.

[pmatilai]

Oh, right. I missed the implications of the newly added filtering in the loop and that of course changes the landscape a lot, so taking that part back.

As for the keyptr setting - yup, for NOKEY you'll want it and the key set to an explicit NULL, and similarly explicit for the case where we actually use a key value in there. It's not at all obvious what the value of "key" is when it falls out of the loops and all above it.

[pmatilai]

Okay, seeing the latest version: I'd add an explicit "key = NULL" to the "No key" case, and then we can call this done.

[pmatilai]

...and then you don't need to NULL it in the loop.

[ffesti]

Guess it is a matter of taste. The original code just made sure the value of key was valid all the time - NULL or not. Now that we rely on the results vector that is not needed as urgently

[pmatilai]

Matter of taste to a degree, sure, but we completely rely on the results vector for the nokey case anyhow so to me it's clearer that all the nokey logic is then in one spot.

[pmatilai]

Looks fine to me now.