Implement rpmkeys --rebuild #3474
Conversation
lib/keystore.cc
Outdated
| auto macros = rpm::macros(); | ||
| auto [mrc, keyringpath] = macros.expand("%{_keyringpath}"); | ||
| while ('/' == keyringpath.back()) | ||
| keyringpath.pop_back(); |
There was a problem hiding this comment.
Just use expand_path(), it'll do this for you. There's actually bug in this version: it'd crash if the path only contained /'s.
lib/keystore.cc
Outdated
| keyringpath.pop_back(); | ||
| std::string path = rpm::expand_path({rpmtxnRootDir(txn), keyringpath}); | ||
| std::string tmppath = keyringpath + ".tmp/"; | ||
| std::string oldpath = keyringpath + ".rpmold/"; |
There was a problem hiding this comment.
Include the rpmtxnRootDir() in these already, as it is you just need to duplicate that work multiple times in the below, making it harder to follow.
| rpmPubkey key = NULL; | ||
| auto iter = rpmKeyringInitIterator(keys, 0); | ||
| while ((rc == RPMRC_OK) && (key = rpmKeyringIteratorNext(iter))) { | ||
| rc = store->import_key(txn, key, 0, 0); |
There was a problem hiding this comment.
This will stop on first error, which seems contrary to the goal of weeding out bad keys with warning.
There was a problem hiding this comment.
... Also this suggests that there shoudn't perhaps be a separate keystore-specific rebuild operation, ie it should just be handled by the rpmts-level rebuild - if we use the rpmdb backend as a model, that doesn't have a backend-specific rebuild operation either.
lib/rpmts.cc
Outdated
|
|
||
| keystore_openpgp_cert_d ks_opengpg = {}; | ||
| rpmKeyring keyring_openpgp = rpmKeyringNew(); | ||
| ks_opengpg.load_keys(txn, keyring_openpgp); |
There was a problem hiding this comment.
Um. Why should we load up all possible keystore types? Only one can possibly be active at any time, and if there happens to be other info laying around I don't think we want anything to do with it, because who knows what put it there.
lib/rpmts.cc
Outdated
| rpmPubkey key = NULL; | ||
| const unsigned char * pkt = NULL; | ||
| size_t pktlen = 0; | ||
| char *lints = NULL; |
There was a problem hiding this comment.
pkt, pktlen and lints belong inside the while() loop.
|
The above isn't a "full review", just remarks on stuff that immediately caught my eye. An observation, not on this PR but this keystore thing in general: like we discussed, the keystore not having any internal state seemed weird and wrong. In retrospective it indeed is. The keystore should take the keyring path as an optional argument and remember it, macro values changing after initialization "obviously" must not affect an existing keystore. I remember thinking about that but then somehow conflating it with the txn root directory. And this PR pays the price for that mistake - seems obvious now but... oh well. Another, sort of related observation is that if temporary macros like the %_keyringpath here are a common use-case, we could arrange the cleanup to occur automatically with the macro context handle destructor. |
|
But okay, there's no point in doing detailed review while there are higher level questions open. I was expecting to see a similar 1:1 rebuild system with optional conversion as the rpmdb has, but this merges multiple sources into one. In all rpm versions, it's only been possible to have one keystore configured at a time, and if we're pulling in anything else then we're making a decision to trust those keys on behalf of the user (because currently imported == trusted). That seems very wrong. |
|
Well, having multiple keystores can happen quite easily. Technically we will always a rpmdb - so there can be gpg-pubkey packages in there no matter what you configured in the macros file. Also an update of rpm that is changing the keystore might pull in a new keys right away - ending up with keys in two different keystores. I do agree we want to be a bit more conservative with just adding random keys from somewhere. But should at least look for other stores and report them. May be require --force or something to actually use them. |
|
We'll need to sort this out in the ticket, this isn't review territory really. |
Merge the keys directly in the keyring
We need to both delete the old key and add the new one.
Simplify rpmtxnImportPubkey No functional change
Don't make stuffing this into a rpmPubkey object a one-way street.
This will become handy later
Allow checking which one is selected by name.
Give an warning if they contain public keys. This allows the user to detect misconfigurations or missing conversion from one backend to another.
|
OK, added check for dangling pubkeys in old keystores for both normal operation and rebuilding the keystore. Patch is still missing man page and may be docs updates. I'll add those as soon as the CLI is green lighted. |
This rebuildis the public key storage. If _keyring is set to a new value and there are keys left in an old store this fails with an error message. The keys can either be included by using one or more instances of --include_backend or dropped with --drop_backends. Keys are stored in the latest format. Keys no longer accepted are dropped. Resolves: rpm-software-management#3347
|
Updated the rpmkeys man page. |
|
Let's split this into smaller pieces to move it forward. There are useful commits in here that we can merge already, eg I think supporting key merge in the keyring directly makes more sense than hiding it inside the import. Basically, split commits 1-6 to another PR, just a few quick review-remarks:
|
This rebuild the public key storage. If _keyring is set to a new value
keys from old storage are moved there and the old stores are deleted.
Keys are stored in the latest format. Keys no longer accepted are
dropped.
Resolves: #3347