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

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

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

On Thu, 2024-11-28 at 10:36 -0500, Eli Schwartz wrote:
> On 11/28/24 8:10 AM, Michał Górny wrote:
> 
> > > +# 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.

Yeah.  I don't see why anyone would specify a non-working location (such
as openpgp.org).

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

Yeah.  And for a minute, it made me worry that you're using long ids
instead of fingerprints.

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

In fact, this is the kind of test I was originally thinking of -- i.e.
literally refetching the file manually and seeing if the minimal export
differs...

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

Then perhaps you should add an explanatory comment instead of expecting
people to catch that from a command so long it doesn't fit on people's
screens.

-- 
Best regards,
Michał Górny


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