Re: [gentoo-dev] [PATCH v2 1/2] sec-keys.eclass: new eclass

["Michał Górny" <mgorny@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 6647 bytes --]

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?

> +# 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.

> +_sec_keys_set_globals() {
> +	if [[ ${SEC_KEYS_VALIDPGPKEYS[*]} ]]; then
> +		local key fingerprint name loc locations=() remote
> +		for key in "${SEC_KEYS_VALIDPGPKEYS[@]}"; do
> +			fingerprint=${key%%:*}
> +			name=${key#${fingerprint}:}; name=${name%%:*}
> +			IFS=, read -r -a locations <<<"${key##*:}"
> +			[[ ${locations[@]} ]] || locations=(ubuntu)
> +			for loc in "${locations[@]}"; do
> +				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.

> +				esac
> +				SRC_URI+="
> +					${remote} -> openpgp-keys-${name}-${loc}-${PV}.asc
> +				"
> +			done
> +		done
> +	fi
> +}
> +_sec_keys_set_globals
> +unset -f _sec_keys_set_globals
> +
> +IUSE="test"
> +PROPERTIES="test_network"
> +RESTRICT="test"
> +
> +BDEPEND="
> +	app-crypt/gnupg
> +	test? ( app-crypt/pgpdump )
> +"
> +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.

> +# @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?

> +
> +	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.

> +	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.

> +	for server in keys.gentoo.org keys.openpgp.org keyserver.ubuntu.com; do
> +		gpg --refresh-keys --keyserver "hkps://${server}"
> +	done
> +	for key in "${SEC_KEYS_VALIDPGPKEYS[@]}"; do
> +		if [[ ${key##*:} = *github* ]]; then
> +			name=${key#*:}; name=${name%%:*}
> +			wget -qO- https://github.com/${name}.gpg | gpg --import || die
> +		fi
> +	done
> +
> +	for fingerprint in "${SEC_KEYS_VALIDPGPKEYS[@]%%:*}"; do
> +		"${gpg_command[@]}" --export "${fingerprint}" | pgpdump > "${fingerprint}.pgpdump.new" || die
> +		diff -u "${fingerprint}.pgpdump" "${fingerprint}.pgpdump.new" || die "updates available for PGP key: ${fingerprint}"
> +	done
> +
> +}
> +
> +# @FUNCTION: sec-keys_src_install
> +# @DESCRIPTION:
> +# Default src_install override that minifies and exports all PGP public keys
> +# into an ascii-armored keyfile installed to the standard /usr/share/openpgp-keys.
> +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.

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?


-- 
Best regards,
Michał Górny


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 512 bytes --]