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

[Eli Schwartz <eschwartz@gentoo.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7064 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]