Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
On 11/28/24 5:35 AM, Ulrich Müller wrote:
>> On Wed, 27 Nov 2024, Eli Schwartz wrote:
>
>> --- /dev/null
>> +++ b/eclass/sec-keys.eclass
>> @@ -0,0 +1,150 @@
>> +# Copyright 2024 Gentoo Authors
>> +# Distributed under the terms of the GNU General Public License v2
>> +
>> +# @ECLASS: sec-keys.eclass
>> +# @MAINTAINER:
>> +# Eli Schwartz
>> +# @AUTHOR:
>> +# Eli Schwartz
>> +# @SUPPORTED_EAPIS: 8
>> +# @BLURB: Provides a uniform way of handling ebuilds which package PGP key
>> material
>> +# @DESCRIPTION:
>> +# This eclass provides a streamlined approach to finding suitable source
>> material
>> +# for OpenPGP keys used by the verify-sig eclass. Its primary purpose is to
>> permit
>> +# developers to easily and securely package new sec-keys/* packages. The
>> eclass
>> +# removes the risk of developers accidentally packaging malformed key
>> material, or
>> +# neglecting to notice when PGP identities have changed.
>> +#
>> +# To use the eclass, define SEC_KEYS_VALIDPGPKEYS to contain the
>> fingerprint of
>> +# the key and the short name of the key's owner.
>
> Please wrap these comment lines to a line length of 70-ish characters
> for readability.
>
> Also, there should be two spaces after every full stop (except when it's
> followed by a newline), so groff can recognise the sentence end in the
> generated man page.
I usually do 80-ish for readability! Okay, I can do 70 too. :) Thanks
for the tip about the spaces, I don't usually write groff by hand.
Surprising that groff cannot handle this automatically, though.
>> +_sec_keys_set_globals() {
>> +if [[ ${SEC_KEYS_VALIDPGPKEYS[*]} ]]; then
>
> Why is the if needed? If the array is empty, the following for loop
> won't execute.
Not sure, perhaps an artifact of a previous revision that had different
handling. Let's remove it.
>> +printf '%s\n' "${imported_keys[@]}" | sort > imported_keys.list || die
>> +printf '%s\n' "${SEC_KEYS_VALIDPGPKEYS[@]%%:*}" | sort >
>> allowed_keys.list || die
>
> Maybe create these files in ${T} instead?
I'm not sure this is an important distinction. It's the main thing the
package works on. I could put GNUPGHOME in ${T} as well, if you like? :)
But keeping it in ${WORKDIR} makes it more straightforward for people to
look at manually when a failed build happens. And that's important when
dealing with the primary logic of a package (there's no source code to
compile here).
--
Eli Schwartz
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
> On Wed, 27 Nov 2024, Eli Schwartz wrote:
> --- /dev/null
> +++ b/eclass/sec-keys.eclass
> @@ -0,0 +1,150 @@
> +# Copyright 2024 Gentoo Authors
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: sec-keys.eclass
> +# @MAINTAINER:
> +# Eli Schwartz
> +# @AUTHOR:
> +# Eli Schwartz
> +# @SUPPORTED_EAPIS: 8
> +# @BLURB: Provides a uniform way of handling ebuilds which package PGP key
> material
> +# @DESCRIPTION:
> +# This eclass provides a streamlined approach to finding suitable source
> material
> +# for OpenPGP keys used by the verify-sig eclass. Its primary purpose is to
> permit
> +# developers to easily and securely package new sec-keys/* packages. The
> eclass
> +# removes the risk of developers accidentally packaging malformed key
> material, or
> +# neglecting to notice when PGP identities have changed.
> +#
> +# To use the eclass, define SEC_KEYS_VALIDPGPKEYS to contain the fingerprint
> of
> +# the key and the short name of the key's owner.
Please wrap these comment lines to a line length of 70-ish characters
for readability.
Also, there should be two spaces after every full stop (except when it's
followed by a newline), so groff can recognise the sentence end in the
generated man page.
> +#
> +# @EXAMPLE:
> +# Example use:
> +#
> +# @CODE
> +# SEC_KEYS_VALIDPGPKEYS=(
> +#'4EC8A4DB7D2E01C00AF36C49E5C587B5E286C65A:jsmith:github'
> +# )
> +#
> +# inherit sec-keys
> +# @CODE
> +
> +case ${EAPI} in
> + 8) ;;
> + *) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
> +esac
> +
> +if [[ ! ${_SEC_KEYS_ECLASS} ]]; then
> +_SEC_KEYS_ECLASS=1
> +
> +inherit edo
> +
> +# @ECLASS_VARIABLE: SEC_KEYS_VALIDPGPKEYS
> +# @PRE_INHERIT
> +# @DEFAULT_UNSET
> +# @DESCRIPTION:
> +# Mapping of fingerprints, name, and optional location of PGP keys to
> include,
> +# separated by colons. The allowed values for a location are:
> +#
> +# - 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)
> +#
> +# - none -- do not add to SRC_URI, the ebuild will provide a custom
> download location
> +_sec_keys_set_globals() {
> + if [[ ${SEC_KEYS_VALIDPGPKEYS[*]} ]]; then
Why is the if needed? If the array is empty, the following for loop
won't execute.
> + 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
> + 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}";;
> +
> + esac
> + SRC_URI+="
> + ${remote} ->
> openpgp-keys-${name}-${loc}-${PV}.asc
> + "
> + done
> + done
> + fi
> +}
> +_sec_keys_set_globals
> +unset -f _sec_keys_set_globals
> +
> +BDEPEND="app-crypt/gnupg"
> +S=${WORKDIR}
> +
> +LICENSE="public-domain"
> +SLOT="0"
> +
> +
> +# @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
> + # 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)
> +
> + printf '%s\n' "${imported_keys[@]}" | sort > imported_keys.list || die
> + printf '%s\n' "${SEC_KEYS_VALIDPGPKEYS[@]%%:*}" | sort >
> allowe
Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
On 11/27/24 4:12 PM, Michał Górny wrote: > On Wed, 2024-11-27 at 15:30 -0500, Eli Schwartz wrote: >> The current state of verify-sig support is a bit awkward. We rely on >> validating distfiles against a known trusted keyring, but creating the >> known trusted keyring is basically all manual verification. We somehow >> decide an ascii armored key is good enough without any portage >> assistance, then arrange to download it and trust it by Manifest hash. >> How do we know when updating a key is actually safe? >> >> This eclass handles the problem in a manner inspired in part by pacman. >> We require an eclass variable that lists all permitted PGP fingerprints, >> and the eclass is responsible checking that list against the keys we >> will install. It comes with a mechanism for computing SRC_URI for a >> couple of well known locations, or you can append your own in the >> ebuild. > > How about adding a src_test() that would check if the key needs bumping, > i.e. if an online update triggers any meaningful changes? This is a really nice suggestion. I used Sam's tip about pgpdump, so that we print a diff after the online update, and fail if diff produces a diff. We use a cleaned and minimized version of the key, so it will only show/trigger on changes to the uid or self-sig packets, which isn't exactly the same as "meaningful changes". For example, running the tests on the gnutls keyring in the second patch, Daiki's key has been updated on Ubuntu with an additional expiry date change for a secondary uid, which may be meaningful in certain senses but we can validate it just fine using the primary uid. -- Eli Schwartz OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
On 11/27/24 4:57 PM, Sam James wrote:
> Eli Schwartz writes:
>> +# @EXAMPLE:
>> +# Example use:
>> +#
>> +# @CODE
>> +# SEC_KEYS_VALIDPGPKEYS=(
>> +# '4EC8A4DB7D2E01C00AF36C49E5C587B5E286C65A:jsmith:github'
>> +# )
>
> Can you expand the example(s) here maybe with some comments in the array
> to help people see when it might be suitable to use e.g. none with a mix
> of github?
Sure, good idea.
>> +# @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
>
> Is there any value in using gemato's gpg-wrap for this function?
I don't think so. The main use case for gemato that I see is it
automatically entering a tempdir context based on a keyfile. We need to
support multiple keyfiles, including ebuild-specified SRC_URI that may
not be ascii-armored and cannot be concatenated together, which means in
order to get to a point where gpg-wrap can be used to run one-off
commands using a keyfile in which gemato wraps the creation of a
keyring... we've basically done all the work we actually wanted to do.
>> +local extra_keys=($(comm -23 imported_keys.list allowed_keys.list ||
>> die))
>> +local missing_keys=($(comm -13 imported_keys.list allowed_keys.list ||
>> die))
>
> Any reason to not readarray this instead?
The files each contain a list of words (PGP fingerprint, consisting of
characters matching [0-9A-F] and nothing else), with the only whitspace
in the file being newline characters.
Both readarray and command substitution tokenize this the same. I'm not
sure it particularly matters which one to use, but command substitution
can be done on one line (local variable=($(command || die)) ) whereas
readarray requires two lines (local -a variable; readarray -t varlist <
<(command || die) ) and you have to remember to use -t and -a.
I don't think readarray provides any functionality we need here.
>> +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 || die
>
> No need for the die here.
Right, I probably forgot to remove this when I switched to edo.
--
Eli Schwartz
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
Eli Schwartz writes:
> The current state of verify-sig support is a bit awkward. We rely on
> validating distfiles against a known trusted keyring, but creating the
> known trusted keyring is basically all manual verification. We somehow
> decide an ascii armored key is good enough without any portage
> assistance, then arrange to download it and trust it by Manifest hash.
> How do we know when updating a key is actually safe?
>
> This eclass handles the problem in a manner inspired in part by pacman.
> We require an eclass variable that lists all permitted PGP fingerprints,
> and the eclass is responsible checking that list against the keys we
> will install. It comes with a mechanism for computing SRC_URI for a
> couple of well known locations, or you can append your own in the
> ebuild.
>
> Key rotations, both expected and malicious, are easily detected by
> checking the git log for changes to declared finterprints in a bump. The
s/finterprints/fingerprints/
> former can be rationalized in the commit message. So can the latter, but
> in most cases those will be rejected during peer review.
>
> Signed-off-by: Eli Schwartz
> ---
> eclass/sec-keys.eclass | 150 +
> 1 file changed, 150 insertions(+)
> create mode 100644 eclass/sec-keys.eclass
>
I'm actually pleasantly surprised by how elegant this is -- which is no
reflection on you, just I sort of assumed the implementation would have
to be a bit gnarlier (not sure why).
One thing I really want to solve in general is key refreshes, given we
have no "src_fetch", users can't opt in to key refreshes even with
verify-sig.eclass unless network-sandbox is off, but that's a separate
problem. mgorny's suggestion wrt src_test would allow us to even
tinderbox needed refreshes which somewhat solves that problem too.
I like it.
> diff --git a/eclass/sec-keys.eclass b/eclass/sec-keys.eclass
> new file mode 100644
> index ..95e1b4a92c0e
> --- /dev/null
> +++ b/eclass/sec-keys.eclass
> @@ -0,0 +1,150 @@
> +# Copyright 2024 Gentoo Authors
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: sec-keys.eclass
> +# @MAINTAINER:
> +# Eli Schwartz
> +# @AUTHOR:
> +# Eli Schwartz
> +# @SUPPORTED_EAPIS: 8
> +# @BLURB: Provides a uniform way of handling ebuilds which package PGP key
> material
> +# @DESCRIPTION:
> +# This eclass provides a streamlined approach to finding suitable source
> material
> +# for OpenPGP keys used by the verify-sig eclass. Its primary purpose is to
> permit
> +# developers to easily and securely package new sec-keys/* packages. The
> eclass
> +# removes the risk of developers accidentally packaging malformed key
> material, or
> +# neglecting to notice when PGP identities have changed.
> +#
> +# To use the eclass, define SEC_KEYS_VALIDPGPKEYS to contain the fingerprint
> of
> +# the key and the short name of the key's owner.
> +#
> +# @EXAMPLE:
> +# Example use:
> +#
> +# @CODE
> +# SEC_KEYS_VALIDPGPKEYS=(
> +#'4EC8A4DB7D2E01C00AF36C49E5C587B5E286C65A:jsmith:github'
> +# )
Can you expand the example(s) here maybe with some comments in the array
to help people see when it might be suitable to use e.g. none with a mix
of github?
> +#
> +# inherit sec-keys
> +# @CODE
> +
> +case ${EAPI} in
> + 8) ;;
> + *) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
> +esac
> +
> +if [[ ! ${_SEC_KEYS_ECLASS} ]]; then
> +_SEC_KEYS_ECLASS=1
> +
> +inherit edo
> +
> +# @ECLASS_VARIABLE: SEC_KEYS_VALIDPGPKEYS
> +# @PRE_INHERIT
> +# @DEFAULT_UNSET
> +# @DESCRIPTION:
> +# Mapping of fingerprints, name, and optional location of PGP keys to
> include,
> +# separated by colons. The allowed values for a location are:
> +#
> +# - 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)
> +#
> +# - none -- do not add to SRC_URI, the ebuild will provide a custom
> download location
> +_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
> + 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}";;;
> +
Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
Michał Górny writes: > On Wed, 2024-11-27 at 15:30 -0500, Eli Schwartz wrote: >> The current state of verify-sig support is a bit awkward. We rely on >> validating distfiles against a known trusted keyring, but creating the >> known trusted keyring is basically all manual verification. We somehow >> decide an ascii armored key is good enough without any portage >> assistance, then arrange to download it and trust it by Manifest hash. >> How do we know when updating a key is actually safe? >> >> This eclass handles the problem in a manner inspired in part by pacman. >> We require an eclass variable that lists all permitted PGP fingerprints, >> and the eclass is responsible checking that list against the keys we >> will install. It comes with a mechanism for computing SRC_URI for a >> couple of well known locations, or you can append your own in the >> ebuild. > > How about adding a src_test() that would check if the key needs bumping, > i.e. if an online update triggers any meaningful changes? Ooh, I like this idea. We could print a pgpdump diff.
Re: [gentoo-dev] [PATCH 1/2] sec-keys.eclass: new eclass
On Wed, 2024-11-27 at 15:30 -0500, Eli Schwartz wrote: > The current state of verify-sig support is a bit awkward. We rely on > validating distfiles against a known trusted keyring, but creating the > known trusted keyring is basically all manual verification. We somehow > decide an ascii armored key is good enough without any portage > assistance, then arrange to download it and trust it by Manifest hash. > How do we know when updating a key is actually safe? > > This eclass handles the problem in a manner inspired in part by pacman. > We require an eclass variable that lists all permitted PGP fingerprints, > and the eclass is responsible checking that list against the keys we > will install. It comes with a mechanism for computing SRC_URI for a > couple of well known locations, or you can append your own in the > ebuild. How about adding a src_test() that would check if the key needs bumping, i.e. if an online update triggers any meaningful changes? -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
