OK, I like the way this works. There are a lot of problems on the current master that are squashed by this and other commits in Dan's git (the "working" branch), so to avoid duplication I'll try to focus on the revision currently in Dan's "working" tree (last commit is this patch, b3581d). I may have misread some things, so this could be wrong: 1) The signature checking in _alpm_pkg_load_internal is messed up. In pacman's master, setting the signature check level to "optional" causes checks to succeed even if: - GPGME fails to initalize - sig isn't valid base64 encoding or .sig file can't be opened - various GPGME operations fail - the encoded PGP information doesn't actually contain a signature(?) - missing pubkey 2) In Dan's git, it's the same as the above, but now "no signature" is an automatic failure, and "bad signature" is explicitly checked for (also an automatic failure). 3) In alpm_db_update, setting the signature check level to "optional" causes pacman to attempt to download a .sig for a database; if it can't download one, it will still accept the database (vs. continuing to next mirror for "always"). Other than that, there is no difference between "optional" and "always"; this isn't bad, but we might want to do the "check 3 servers and then give up" thing for PM_SIG_DATABASE_OPTIONAL. It's not really KISS (probably extra baggage in the documentation too), so IDK. On an unrelated note, Dan killed a bug related to stale signatures in the "optional" case with the patch "Do database signature checking at load time" (bd1cf5 in his "working" branch), but, as Allan mentions in his notes, the new database still overwrites the old one in the "always" case (because the check-for-signature-existence happens after the new database is downloaded). 4) Dan's git adds signature checks for databases, which aren't in master. Although, I don't know if _alpm_db_register_sync() and alpm_db_update() necessarily the best places to do the signature check: since we're already doing general integrity checking in sync_db_populate (using the internal checksums embedded in various archive formats), doing a signature check at the top of that function might be better, but I don't really grok the code flow yet. _alpm_db_register_sync() (and thus the signature check) seems to get called every time pacman is invoked, which I don't think is desirable. So, _alpm_pkg_load_internal needs some attention, but we should decide what "optional" means first. I thought it would encompass the "no signature" and "missing pubkey" cases, but it seems to be backwards. 5) As far as I can tell, there should be a bug (which I cannot produce) in alpm_pkg_load() because it passes base64_sig to _alpm_pkg_load_internal() as NULL, so _alpm_log(handle, PM_LOG_DEBUG, "base64_sig: %s\n", base64_sig); should segfault; also, _alpm_gpgme_checksig() should balk at the NULL base64_sig, but I was too lazy at the time of writing to throw it into gdb, and this line didn't even seem to get printed when I ran ./pacman --debug --conf ~/pacman.conf -Q blah |& grep -i base64_sig On Tue, Jun 14, 2011 at 3:26 PM, Dan McGee <dan@archlinux.org> wrote:
To get any checking at all, at least one of PM_CHECK_PACKAGE or PM_CHECK_DATABASE must be flipped on. These can be made optional on an independent basis.
I think these became PM_SIG_PACKAGE and PM_SIG_DATABASE in your patch, but I like PM_SIG_CHECK_PACKAGE and PM_SIG_CHECK_DATABASE better (they're clearer but a bit long; perhaps _PKG and _DB would work since those abbreviations are also used a lot elsewhere; corresponding PM_SIG_PKG_OPTIONAL and PM_SIG_DB_OPTIONAL would be also be good).
+int alpm_option_set_default_siglevel(pmhandle_t *handle, pmsiglevel_t level); ... pmdb_t *alpm_db_register_sync(pmhandle_t *handle, const char *treename, - pgp_verify_t check_sig); + pmsiglevel_t level);
I like "pmsiglevel_t level" for cases like the first where it's specifically dealing with signing, but "pmsiglevel_t siglevel" for cases like the second where it's only a small part of a bigger function. Not a huge issue either way. As a self-reminder, config flags/directives still need to be defined and handled. I wanted to respond earlier, but school's kept me busy, and there's been a bunch of patches/commits, so I wanted to make sure I'm understanding everything that's been happening recently. I'm not done looking this over, but it's giving me a headache right now; anyway, I think I'll just write a bunch of pactests instead of digging through the code. Can we let this patch sit for a couple of days, and then go ahead and apply it to master (possibly with the CHECK/PKG/DB/siglevel suggestions)? It's not functionally complete, but it serves to refactor the code: it (apparently) doesn't break anything, and allows future work to go forth without annoying merges due to mixed conventions. -Kerrick Staley