Add an API for permanently removing public keys from keystore

[pmatilai]

We have rpmtsImportPubkey() but we lack a counterpart to remove keys. It's been possible to remove gpg-pubkeys through the normal package transaction API, but there's no support for removing keys from the fs keystore at all. We should add something like rpmtsDeletePubkey() to delete keys from the keystore - hooked to rpmts, because that's where rpmtsImportPubkey() is and changing that is just too painful, but also that gives us transaction locking so there's basically no need for further locking in the keystore level.

[pmatilai]

After poking around a bit, a direct delete counterpart rpmtsImportPubkey(rpmts, ...) doesn't make much sense. We'll want these to take rpmtxn as the first argument, so the caller does the locking and then you can process multiple items at once. So I'm planning on adding

• rpmRC rpmtxnImportPubkey(rpmtxn txn, const unsigned char * pkt, size_t pktlen)
• rpmRC rpmtxnDeletePubkey(rpmtxn txn, const char *keyid) /* takes hex keyid or fingerprint */

... and then rpmtsImportPubkey() becomes just a compatibility wrapper around the txn-version

[pmatilai]

Work in progress-tree: https://github.com/pmatilai/rpm/tree/delkey

What's primarily missing there is tests for fs keyring delete, and the bigger issue: while doing this I realized we seem to lack tests for the key merge functionality which this touches.

[pmatilai]

I guess an AC would be something to the tune of:

• add transaction layer API for permanently removing keys from transaction keystore:
  • rpmRC rpmtxnDeletePubkey(rpmtxn txn, const char *keyid)
• the API accepts an keyid or fingerprint
• the API returns a code to indicate operation success/failure

Feel free to refine (@ffesti?)

[ffesti]

I wonder if this should pass a rpmPubkey instead and leave the keyid to rpmPubkey conversion to the cli layer. FYI I do have a function that executes a callback for argv in my rpmkeys --list patch that can be used for deletion, too. If we want to push this down a layer it can move into rpmcli or even further down to be used in rpmtxnDeletePubkey

[pmatilai]

From the PR, to keep discussion in one place:

As said in https://github.com/rpm-software-management/rpm/issues/3338 I don't like the idea of passing the keyid to the backend. This even accepts short keyids which we are trying to get rid of. I also don't like that it makes use of the fact that the storage backends are based on the key ids. For one we want to change that. It also leaks this implementation detail into the API a bit too much for my taste.

Fingerprints are the main handle the public keys are known by, so I don't think it's an "implementation detail" any more than the fact that packages are accessed by their nvr.

The fact that it now accepts the short keyid is because there's just no other way to do it currently - that's ticket #3360, and I didn't want to drag that too into this PR. I brought this mismatch up in the discussions around changing the output to the fingerprint.

The thought of passing rpmPubkey there did occur to me too, but right now there's no pubkey that can be passed there, that road is blocked on the keyring iterator. There are couple of reasons I dislike that: you need to get those keys somewhere, so you need to have the fingerprint/keyid visible on some public API, or you need to do a linear search on an iterator. Which doesn't seem that great either. And deleting stuff you're iterating on gets tricky, although if its iterating the keyring and not the keystore that's not so much an issue. The other thing is that constructing something just to delete it seems kinda strange - and if there's a bug or the key gets malformed, you might no longer be able to delete such a key then.

[pmatilai]

One note here that the rpmRC "mapping" in the PR is super ugly for the NOKEY return. I wonder if we shouldn't just add RPMRC_INVALID for this kind of case, we have these rc's all over the place and this can't be the only one...

[pmatilai]

On a more general note: I think we'll need to accept that there's just so many missing or broken interdependent things around this stuff that we can't hope to land everything in the final shape.

The approach in the PR is AFAICS pretty much the best we can do just now (possible bugs and such not counting of course), and that step unlocks further steps.

[ffesti]

Yeah, I am fine with doing things step by step as long as we don't fall into love with the API until we got this all sorted out.

[ffesti]

OK, I just noticed this doesn't actually support passing long keyids or fingerprints. We should probably add that and tests for it.

[pmatilai]

It allows long keyids and fingerprints, but it can't handle them because the keystore doesn't have a way to fetch by them. The keystore needs to move to fingerprints first.

As for the API, I'm not exactly in love with it either, but this is quite literally the key counterpart of rpmErase() - which takes a package N[-V[-R[.A]]]] label and works it from there. And for keys, the fingerprint is that label. I intend us to have a proper public API for the actual keystore eventually, but this allows us to move on with minimum exposed parts at this time.

[ffesti]

Well, just use the last 8 hex bytes (4 of the binary) of the fingerprint / long keyid.

[pmatilai]

It doesn't make sense to try hack around the underlying storage, it would only make for very weird behavior for little benefit. e'll handle this in #3360.