On 11/28/24 8:10 AM, Michał Górny wrote: > On Wed, 2024-11-27 at 23:32 -0500, Eli Schwartz wrote: >> +# @ECLASS_VARIABLE: SEC_KEYS_VALIDPGPKEYS >> +# @PRE_INHERIT >> +# @DEFAULT_UNSET >> +# @DESCRIPTION: >> +# Mapping of fingerprints, name, and optional location of PGP keys to >> include, > > So "location" or "locations", plural?
Fixed. >> +# separated by colons. The allowed values for a location are: >> +# >> +# - gentoo -- fetch key by fingerprint from https://keys.gentoo.org >> +# >> +# - github -- fetch key from github.com/${name}.pgp >> +# >> +# - openpgp -- fetch key by fingerprint from https://keys.openpgp.org >> +# >> +# - ubuntu -- fetch key by fingerprint from http://keyserver.ubuntu.com >> (the default) > > I'd go without a default. Typing 6 more letters doesn't cost anything, > and makes the contents more consistent. Also saves us from regretting > having chosen a bad default in the future. > >> +# >> +# - none -- do not add to SRC_URI, the ebuild will provide a custom >> download location > > Perhaps "manual"? "None" sounds like there would be no key at all. Maybe I could just document as a recommendation to use ubuntu. Other sources are likely to be extremely unreliable, unfortunately. openpgp.org only works if the key owner manually verifies their email, for example. >> + case ${loc} in >> + gentoo) >> remote="https://keys.gentoo.org/pks/lookup?op=get&search=0x${fingerprint}";;; >> + github) >> remote="https://github.com/${name}.gpg";;; >> + openpgp) >> remote="https://keys.openpgp.org/vks/v1/by-fingerprint/${fingerprint}";;; >> + ubuntu) >> remote="https://keyserver.ubuntu.com/pks/lookup?op=get&search=0x${fingerprint}";;; >> + # provided via manual SRC_URI >> + none) continue;; >> + *) die "${ECLASS}: unknown PGP key >> remote: ${loc}";; >> + > > Stray empty line. Fixed. >> +S=${WORKDIR} >> + >> +LICENSE="public-domain" >> +SLOT="0" > > Please keep ebuildy variables in the standard/skel order, or at least > as close to it as you can get. Fixed. >> +# @FUNCTION: sec-keys_src_compile >> +# @DESCRIPTION: >> +# Default src_compile override that imports all public keys into a keyring, >> +# and validates that they are listed in SEC_KEYS_VALIDPGPKEYS. >> +sec-keys_src_compile() { >> + local -x GNUPGHOME=${WORKDIR}/gnupg >> + mkdir -m700 -p "${GNUPGHOME}" || die >> + >> + pushd "${DISTDIR}" >/dev/null || die >> + gpg --import ${A} || die >> + popd >/dev/null || die >> + >> + local line imported_keys=() found=0 >> + while IFS=: read -r -a line; do >> + if [[ ${line[0]} = pub ]]; then > > Please use '==' in ebuilds and eclasses. ... >> + # new key >> + found=0 >> + elif [[ ${found} = 0 && ${line[0]} = fpr ]]; then >> + # primary fingerprint >> + imported_keys+=("${line[9]}") >> + found=1 >> + fi >> + done < <(gpg --batch --list-keys --keyid-format=long --with-colons || >> die) > > Why do you need --keyid-format? You're using fingerprints only, aren't > you? I'm used to it mattering in various contexts and added it instinctively. You're right, it doesn't do anything here. >> + >> + printf '%s\n' "${imported_keys[@]}" | sort > imported_keys.list || die >> + printf '%s\n' "${SEC_KEYS_VALIDPGPKEYS[@]%%:*}" | sort > >> allowed_keys.list || die >> + >> + local extra_keys=($(comm -23 imported_keys.list allowed_keys.list || >> die)) >> + local missing_keys=($(comm -13 imported_keys.list allowed_keys.list || >> die)) >> + >> + if [[ ${#extra_keys[@]} != 0 ]]; then >> + die "too many keys found. Suspicious keys: ${extra_keys[@]}" > > The first sentence is not capitalized. Fixed. >> + fi >> + if [[ ${#missing_keys[@]} != 0 ]]; then >> + die "too few keys found. Unavailable keys: ${missing_keys[@]}" >> + fi >> +} >> + >> + >> +sec-keys_src_test() { >> + local -x GNUPGHOME=${WORKDIR}/gnupg >> + local key fingerprint name server >> + local gpg_command=(gpg --export-options export-minimal) >> + >> + for fingerprint in "${SEC_KEYS_VALIDPGPKEYS[@]%%:*}"; do >> + "${gpg_command[@]}" --export "${fingerprint}" | pgpdump > >> "${fingerprint}.pgpdump" || die >> + done >> + >> + # Best-effort attempt to check for updates. keyservers can and usually >> do >> + # fail for weird reasons, (such as being unable to import a key without >> a >> + # uid) as well as normal reasons, like the key being exclusive to a >> + # different keyserver. this isn't a reason to fail src_test. > > Well, I dare say that if refreshing against the server specified > as the reference source fails, that would count as a reason to fail. > Consider the case of someone removing a compromised key instead > of revoking it. This doesn't test a useful property. People cannot "remove" compromised keys from a keyserver to begin with. If they did, then checking to build the package with GENTOO_MIRRORS= DISTDIR=$(mktemp -d) is a significantly more useful test. Removing a key for whatever reason, doesn't tell you why it was removed, or even who removed it. It is also not how the PGP standard says you are supposed to handle a *compromised* key. It's not like GnuPG will delete keys from your keyring if the server doesn't possess it anymore... there is actually no such thing as a user of PGP that would be correctly served by someone removing a compromised key in the hopes that those users would interpret it as an indicator of compromise. In exchange for no assurances whatsoever, the code would become a lot more complicated. As I already said, gpg exiting with something other than 0 for success can mean many things and there's no good way to figure out what it did in fact mean. I'm going to need a better argument if you want me to change this. >> +sec-keys_src_install() { >> + local -x GNUPGHOME=${WORKDIR}/gnupg >> + local fingerprint >> + local gpg_command=(gpg --no-permission-warning --export-options >> export-minimal) >> + >> + for fingerprint in "${SEC_KEYS_VALIDPGPKEYS[@]%%:*}"; do >> + local uids=() >> + mapfile -t uids < <("${gpg_command[@]}" --list-key >> --with-colons ${fingerprint} | awk -F: '/^uid/{print $10}' || die) >> + edo "${gpg_command[@]}" "${uids[@]/#/--comment=}" --export >> --armor "${fingerprint}" >> ${PN#openpgp-keys-}.asc >> + done > > That looks like something you could do in src_compile() already. Perhaps. But it felt like exporting keys is work that is conceptually part of installing, in much the way that running a meson project's `meson install` step does more than just copy files into ${D} -- it also processes those files in order to do things like patch the rpath. I guess I am not too attached to either approach. > Also, I'm confused by the purpose of this whole logic. After all, you > have already verified that there are no stray keys in the keyring, > right? So why not just export the whole thing? Because this is doing additional steps that aren't just exporting the whole thing? -- Eli Schwartz
OpenPGP_signature.asc
Description: OpenPGP digital signature
