[pacman-dev] [PATCH] WIP: possible new siglevel layout

[Kerrick Staley]

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 at 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