Pass rpmPubkey instance to rpmtxnDeletePubkey #3374
Conversation
| @@ -359,7 +360,7 @@ rpmRC rpmtxnImportPubkey(rpmtxn txn, const unsigned char * pkt, size_t pktlen); | |||
| * RPMRC_NOKEY on invalid keyid | |||
| * RPMRC_FAIL on other failure | |||
| */ | |||
| rpmRC rpmtxnDeletePubkey(rpmtxn txn, const char *keyid); | |||
| rpmRC rpmtxnDeletePubkey(rpmtxn txn, rpmPubkey key); | |||
There was a problem hiding this comment.
I'm okay with adding an alternative API for this that takes a pubkey, and I'm okay with using this name for it even, but don't remove the version that takes a fingerprint. I explained why in the PR that added it.
There was a problem hiding this comment.
Actually, scratch that.
Going forward, it is a requirement for rpmkeys (and the library part) to be able to delete keys without being able to construct a pubkey out of it. I've seen enough crypto related failures in the last few years that this is just non-negotiable. But, with what we have now, we have to load it into the keyring first anyhow. So okay for changing the argument to rpmPubkey, we'll add another interface around the lower-level keystore later, once we have that and an iterator for the contents.
There was a problem hiding this comment.
OK, this here is the minimal approach. It keeps the old function as rpmtxnDeletePubkeyByID. We can smush this into a different shape once we now what to do in the backend.
There was a problem hiding this comment.
Ok, but see #3375 (comment) - I drop my case on the fingerprint/keyid API - been insisting on it for what is a wrong reason.
lib/rpmchecksig.cc
Outdated
| @@ -290,3 +291,61 @@ int rpmcliVerifySignatures(rpmts ts, ARGV_const_t argv) | |||
| rpmKeyringFree(keyring); | |||
| return res; | |||
| } | |||
|
|
|||
| int cliMatchPubkeys(rpmts ts, ARGV_const_t args, int callback(rpmPubkey, void*), void * userdata) | |||
There was a problem hiding this comment.
Let's keep this baby inside rpmkeys for now.
There was a problem hiding this comment.
This kinda needs rpmIsValidHex whihc currently is internal only. Sure that's no reason to add this to the API. Still it needs some sort of solution.
There was a problem hiding this comment.
A low-cost workaround is to drop RPM_GNUC_INTERNAL from rpmIsValidHex(). It'll show up in the library ABI but is basically uncallable because it doesn't have a public header, and pretty harmless as such. We have quite a few such items, for similar reasons.
It'll just need an extra directive for rpmkeys in the cmake file to allow it to include from lib/, see rpmlua/rpmgraph for examples on that.
There was a problem hiding this comment.
I just added my own loop there. Yes, it is sortof duplicated code. But fiddling around with compiler directives doesn't really reduce the complexity either.
ffesti
force-pushed
the
deletekey
branch
2 times, most recently
from
9291345 to
5101c63
Compare
include/rpm/rpmts.h
Outdated
| * @param key public key | ||
| * @return RPMRC_OK on success | ||
| * RPMRC_NOTFOUND if key not found | ||
| * RPMRC_NOKEY on invalid keyid |
There was a problem hiding this comment.
You can drop the NOKEY return, if it takes a pubkey then we already know it's a key.
Getting rid of that ugly kludge NOKEY == INVALID mapping is actually another reason to just drop the ByID variant.
Make the output consistent and use RPMLOG_ERR
Get this in sync with rpm --delete for now.
Pass in rpmts instead of only a keyring Move userdata pointer to end of signature and make it optional Check for invalid character
Use the matchingKeys() in rpmkeys to acquire those rpmPubkey instances. Use EXIT_FAILURE as exit code for rpmkeys --delete instead of the count of errors.
This is a follow up to #3369 and #3368.
The new cliMatchPubkeys() will become handy for #3366, too