| Message ID | 20220608173227.170944-1-diabonas@archlinux.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | libmakepkg/integrity: handle PGP signature files containing multiple signatures | expand |
Nice spot and great patch :) Review disclaimer: patch not field tested yet On 6/8/22 19:32, Jonas Witschel wrote: > PGP signature files can contain multiple signatures from different keys. In > this case, verification is considered to be successful if *all* signatures are > valid, compare e.g. the behaviour of the gpgv verification tool. No hard feelings here at all, but my thoughts are as follows: I'm not too sure we really want to mimic gpgv behavior. Validating all signatures sounds great in theory, but the only real security guarantee we can give with the current control mechanisms and options in makepkg is basically "source has any signature available in validpgpkeys". As we have no constraints to specify a signature threshold, there is no way for makepkg to consider tree, two or one available signature as more or less trustworthy. For instance a rogue maintainer can just distribute whatever they want using a single trusted signature which makepkg will happily consume. Hence the only guarantee we can provide currently is to have `any` signature instead of `all`. While this is the case, this may lead to potential issues like "random" secondary signatures or potentially even old superseded keys that are still used as a multi signature which we don't actually wanna forcefully trust anymore. Taking this into account, I'd suggest we go with an `any` approach instead of `all` for the time being. If makepkg ever gets finer trust control per source, such adjustments should be reflected here as well. > > parse_gpg_statusfile() is currently not equipped to handle the gpg output for > such signature files: it assumes that the file contains only one signature and > therefore overwrites the verification results of previous signatures, > effectively only considering the last signature in the file. > > Therefore the logic needs to be refined so that $success and $trusted are only > true if all signatures are valid and trusted, respectively. We also need to > store the verification results per key (in order to present more specific error > messages) and all the validated fingerprints (in order to check whether they > are in validpgpkeys). > > The signatures for the source tarballs of recent GnuPG releases, e.g. version > 2.2.35, are examples of such signature files. > > Signed-off-by: Jonas Witschel <diabonas@archlinux.org> > --- > .../integrity/verify_signature.sh.in | 115 ++++++++++-------- > 1 file changed, 65 insertions(+), 50 deletions(-) > > diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in > index ad5bb66d..4aa14d94 100644 > --- a/scripts/libmakepkg/integrity/verify_signature.sh.in > +++ b/scripts/libmakepkg/integrity/verify_signature.sh.in > @@ -26,6 +26,18 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'} > source "$LIBRARY/util/message.sh" > source "$LIBRARY/util/pkgbuild.sh" > > +# Returns the elements of the first array that are not found in the second > +# array as the third array. All arrays are passed by name. > +set_difference() { I believe a better place in libmakepkg would be to move `set_difference` into `scripts/libmakepkg/util/util.sh.in` next to `in_array`, `is_array` and friends and hence make it an exposed util function. Potentially naming it array instead of set would make sense for such an util function, like `array_diff` or `array_difference`. > + local -n first=$1 > + local -n second=$2 > + local -n result=$3 namerefs yield an error in bash if they are named equal to a variable in the caller scope. For all nameref's I would suggest to choose something very unique, possible with an underscore prefix. I'd maybe go with something like `_first_diff_array`, `_result_diff_array` or similar vars. > + result=() > + for element in "${first[@]}"; do > + ! in_array "$element" "${second[@]}" && result+=("$element") > + done > +} > + > check_pgpsigs() { > (( SKIPPGPCHECK )) && return 0 > ! source_has_signatures && return 0 > @@ -56,48 +68,53 @@ check_pgpsigs() { > fi > > # these variables are assigned values in parse_gpg_statusfile > - success=0 > - status= > - pubkey= > - fingerprint= > + success= > + declare -A status > + fingerprints=() > trusted= > parse_gpg_statusfile "$statusfile" > if (( ! $success )); then > printf '%s' "$(gettext "FAILED")" >&2 > - case "$status" in > - "missingkey") > - printf ' (%s)' "$(gettext "unknown public key") $pubkey" >&2 > - ;; > - "revokedkey") > - printf " ($(gettext "public key %s has been revoked"))" "$pubkey" >&2 > - ;; > - "bad") > - printf ' (%s)' "$(gettext "bad signature from public key") $pubkey" >&2 > - ;; > - "error") > - printf ' (%s)' "$(gettext "error during signature verification")" >&2 > - ;; > - esac > + for pubkey in "${!status[@]}"; do > + case "${status["$pubkey"]}" in > + "missingkey") > + printf ' (%s)' "$(gettext "unknown public key") $pubkey" >&2 > + ;; > + "revokedkey") > + printf " ($(gettext "public key %s has been revoked"))" "$pubkey" >&2 > + ;; > + "bad") > + printf ' (%s)' "$(gettext "bad signature from public key") $pubkey" >&2 > + ;; > + "error") > + printf ' (%s)' "$(gettext "error during signature verification")" >&2 > + ;; > + esac > + done > errors=1 > else > + local invalid_keys > + set_difference fingerprints validpgpkeys invalid_keys > if (( ${#validpgpkeys[@]} == 0 && !trusted )); then > - printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "$fingerprint" >&2 > + printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "${fingerprints[*]}" >&2 > errors=1 > - elif (( ${#validpgpkeys[@]} > 0 )) && ! in_array "$fingerprint" "${validpgpkeys[@]}"; then > - printf "%s (%s %s)" "$(gettext "FAILED")" "$(gettext "invalid public key")" "$fingerprint" >&2 > + elif (( ${#validpgpkeys[@]} > 0 )) && (( ${#invalid_keys[@]} > 0 )); then > + printf "%s (%s %s)" "$(gettext "FAILED")" "$(gettext "invalid public key")" "${invalid_keys[*]}" >&2 > errors=1 > else > printf '%s' "$(gettext "Passed")" >&2 > - case "$status" in > - "expired") > - printf ' (%s)' "$(gettext "WARNING:") $(gettext "the signature has expired.")" >&2 > - warnings=1 > - ;; > - "expiredkey") > - printf ' (%s)' "$(gettext "WARNING:") $(gettext "the key has expired.")" >&2 > - warnings=1 > - ;; > - esac > + for pubkey in "${!status[@]}"; do > + case "${status["$pubkey"]}" in > + "expired") > + printf " (%s $(gettext "the signature by key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2 > + warnings=1 > + ;; > + "expiredkey") > + printf " (%s $(gettext "the key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2 > + warnings=1 > + ;; > + esac > + done > fi > fi > printf '\n' >&2 > @@ -201,59 +218,57 @@ verify_git_signature() { > parse_gpg_statusfile() { > local type arg1 arg6 arg10 > > + success=-1 > + trusted=-1 > while read -r _ type arg1 _ _ _ _ arg6 _ _ _ arg10 _; do > case "$type" in > GOODSIG) > - pubkey=$arg1 > - success=1 > - status="good" > + success=$((success!=0)) > + status["$arg1"]="good" > ;; > EXPSIG) > - pubkey=$arg1 > - success=1 > - status="expired" > + success=$((success!=0)) > + status["$arg1"]="expired" > ;; > EXPKEYSIG) > - pubkey=$arg1 > - success=1 > - status="expiredkey" > + success=$((success!=0)) > + status["$arg1"]="expiredkey" > ;; > REVKEYSIG) > - pubkey=$arg1 > success=0 > - status="revokedkey" > + status["$arg1"]="revokedkey" > ;; > BADSIG) > - pubkey=$arg1 > success=0 > - status="bad" > + status["$arg1"]="bad" > ;; > ERRSIG) > - pubkey=$arg1 > success=0 > if [[ $arg6 == 9 ]]; then > - status="missingkey" > + status["$arg1"]="missingkey" > else > - status="error" > + status["$arg1"]="error" > fi > ;; > VALIDSIG) > if [[ $arg10 ]]; then > # If the file was signed with a subkey, arg10 contains > # the fingerprint of the primary key > - fingerprint=$arg10 > + fingerprints+=("$arg10") > else > - fingerprint=$arg1 > + fingerprints+=("$arg1") > fi > ;; > TRUST_UNDEFINED|TRUST_NEVER) > trusted=0 > ;; > TRUST_MARGINAL|TRUST_FULLY|TRUST_ULTIMATE) > - trusted=1 > + trusted=$((trusted!=0)) > ;; > esac > done < "$1" > + success=$((success>0)) > + trusted=$((trusted>0)) > } > > source_has_signatures() {
Thank you very much for your review, Levente! See below for my answers to the points you raised. On 2022-06-08 21:15, Levente Polyak wrote: > I'm not too sure we really want to mimic gpgv behavior. Validating all > signatures sounds great in theory, but the only real security guarantee > we can give with the current control mechanisms and options in makepkg > is basically "source has any signature available in validpgpkeys". > > As we have no constraints to specify a signature threshold, there is no > way for makepkg to consider tree, two or one available signature as more > or less trustworthy. For instance a rogue maintainer can just distribute > whatever they want using a single trusted signature which makepkg will > happily consume. Hence the only guarantee we can provide currently is > to have `any` signature instead of `all`. > > While this is the case, this may lead to potential issues like "random" > secondary signatures or potentially even old superseded keys that are > still used as a multi signature which we don't actually wanna forcefully > trust anymore. > > Taking this into account, I'd suggest we go with an `any` approach > instead of `all` for the time being. If makepkg ever gets finer trust > control per source, such adjustments should be reflected here as well. Having thought about this for a bit, I agree with your reasoning and have implemented the "any" logic, which I will post as an updated version of the patch. > I believe a better place in libmakepkg would be to move `set_difference` > into `scripts/libmakepkg/util/util.sh.in` next to `in_array`, `is_array` > and friends and hence make it an exposed util function. > > Potentially naming it array instead of set would make sense for such an > util function, like `array_diff` or `array_difference`. Done. Note that due to the changed logic it is not a "set_difference" any more, but an "arrays_intersect" function because we need to check whether any of the fingerprints is also in validpgpkeys to find a valid signature. > > + local -n first=$1 > > + local -n second=$2 > > + local -n result=$3 > > namerefs yield an error in bash if they are named equal to a variable in > the caller scope. For all nameref's I would suggest to choose something > very unique, possible with an underscore prefix. I'd maybe go with > something like `_first_diff_array`, `_result_diff_array` or similar > vars. The variables are now called _arrays_intersect_{first,second} to avoid naming collisions. Best, Jonas
On 9/6/22 05:15, Levente Polyak wrote: > Nice spot and great patch :) > > Review disclaimer: patch not field tested yet > > > On 6/8/22 19:32, Jonas Witschel wrote: >> PGP signature files can contain multiple signatures from different >> keys. In >> this case, verification is considered to be successful if *all* >> signatures are >> valid, compare e.g. the behaviour of the gpgv verification tool. > > No hard feelings here at all, but my thoughts are as follows: > > I'm not too sure we really want to mimic gpgv behavior. Validating all > signatures sounds great in theory, but the only real security guarantee > we can give with the current control mechanisms and options in makepkg > is basically "source has any signature available in validpgpkeys". > > As we have no constraints to specify a signature threshold, there is no > way for makepkg to consider tree, two or one available signature as more > or less trustworthy. For instance a rogue maintainer can just distribute > whatever they want using a single trusted signature which makepkg will > happily consume. Hence the only guarantee we can provide currently is > to have `any` signature instead of `all`. > > While this is the case, this may lead to potential issues like "random" > secondary signatures or potentially even old superseded keys that are > still used as a multi signature which we don't actually wanna forcefully > trust anymore. > > Taking this into account, I'd suggest we go with an `any` approach > instead of `all` for the time being. If makepkg ever gets finer trust > control per source, such adjustments should be reflected here as well. I have read this argument multiple times and I just don't understand it... Switching to any signature being valid rather than all means we could have two signatures saying "bad file, do not use" and one saying "all is good", and we trust the good one. I would not use the source fine in this case. Allan
On 2022-06-25 15:18, Allan McRae wrote: > > I'm not too sure we really want to mimic gpgv behavior. Validating all > > signatures sounds great in theory, but the only real security guarantee > > we can give with the current control mechanisms and options in makepkg > > is basically "source has any signature available in validpgpkeys". > > > > As we have no constraints to specify a signature threshold, there is no > > way for makepkg to consider tree, two or one available signature as more > > or less trustworthy. For instance a rogue maintainer can just distribute > > whatever they want using a single trusted signature which makepkg will > > happily consume. Hence the only guarantee we can provide currently is > > to have `any` signature instead of `all`. > > > > While this is the case, this may lead to potential issues like "random" > > secondary signatures or potentially even old superseded keys that are > > still used as a multi signature which we don't actually wanna forcefully > > trust anymore. > > > > Taking this into account, I'd suggest we go with an `any` approach > > instead of `all` for the time being. If makepkg ever gets finer trust > > control per source, such adjustments should be reflected here as well. > > I have read this argument multiple times and I just don't understand it... > > Switching to any signature being valid rather than all means we could have > two signatures saying "bad file, do not use" and one saying "all is good", > and we trust the good one. I would not use the source fine in this case. If there is one good and one bad signature, this could mean that one of the two maintainers has gone rogue and uploaded a malicious release, but was not able to produce a valid signature by the other maintainer. However, nothing stops them from just uploading a release that has only a single valid signature made by their own key: there is no way to tell pacman (or gpgv, for that matter) that a release must be signed by both keys, since there is no way to specify a threshold on the number of required signatures. So in this scenario, requiring both signatures to be valid does not offer increased signature benefits since a new release with just a single signature would be recognised as valid just as well. On the other hand, upstreams might be signing releases with an old, deprecated key in addition to the current main signing key. In this case, if the old key gets revoked at some point we would not want to invalidate releases just because they are still signed by the old key, which is what would happen when requiring all signatures to be valid. Even worse, if we require all signatures to be valid, the old key *must* be added to the validpgpkeys array because a release signed by both keys could not be validated without trusting that key. However, a new release signed by only the old key would then also be considered to be valid, see above. So requiring all keys to be valid forces maintainers to add less trusted keys as valid just to make the validation pass. Levente's argument is that we do not gain much from validating both signatures in the first scenario, while in the second case requiring both signatures even makes the situation actively worse. Therefore without being able to specify more detailed key usage policies in the PKGBUILD, trusting any valid signature seems to be a reasonable default, and is also in line with the current approach of trusting *any* of the keys in the validpgpkeys array. One way the situation could be improved is to add a threshold for the number of required valid signatures. In the first scenario, this threshold could be set to 2 in order to always require both signatures to be present and valid. In the second scenario, one would set the threshold to 1 instead and only add the newer key to validpgpkeys, which prevents releases that are only signed by the deprecated key to be considered as valid. However, it is not entirely clear what the default value for this new threshold would be: setting it to the number of keys in the signature file does not make sense since this can arbitrarily vary between releases as shown by the first scenario above. Therefore the only reasonable default threshold would be 1, which exactly corresponds to the approach taken in v2 of the proposed patch to require at least one valid signature. Therefore I would argue that having such a threshold would be more of an improvement that could be based on v2 of the patch rather than a separate implementation: it could be easily achieved by changing arrays_intersect() to return the number of elements in the intersection and to compare that number with a new threshold variable specified in the PKGBUILD (which would default to 1 if unset in order to be backwards compatible). Best, Jonas
On 25/6/22 21:44, Jonas Witschel wrote: <snip> > Therefore without being able to specify > more detailed key usage policies in the PKGBUILD, trusting any valid signature > seems to be a reasonable default, and is also in line with the current approach > of trusting *any* of the keys in the validpgpkeys array. Somewhat offtopic - I'd argue that it is poor packaging to have more validpgpkeys in a PKGBUILD than the key used to verify the source for that particular package version. Lazily just adding more keys to a PKGBUILD and not removing unneeded ones in case they are needed in the future is not ideal. <snip> > Therefore I would argue that having such a threshold would be more of an > improvement that could be based on v2 of the patch rather than a separate > implementation: it could be easily achieved by changing arrays_intersect() to > return the number of elements in the intersection and to compare that number > with a new threshold variable specified in the PKGBUILD (which would default to > 1 if unset in order to be backwards compatible). How about... all signatures from trusted keys (either through PGP web of trust, or via being in validpgpkeys) need to validate? That does not solve the rogue maintainer issue (it is up to a packager to ask why the number of signatures dropped...), but it does address not needing to validate a legacy key. Allan
On 2022-06-25 22:09, Allan McRae wrote: > Somewhat offtopic - I'd argue that it is poor packaging to have more > validpgpkeys in a PKGBUILD than the key used to verify the source for that > particular package version. Lazily just adding more keys to a PKGBUILD and > not removing unneeded ones in case they are needed in the future is not > ideal. We should indeed be more careful in actively removing old keys from validpgpkeys. However some upstreams have multiple active maintainers that "arbitrarily" sign new releases, in which case keeping all of them in the array can be helpful to convey an existing relationship of trust (without having to go through the revision control system to figure out whether the key of a new release has been already used in the past, or whether some other means of establishing a chain of trust is necessary). > How about... all signatures from trusted keys (either through PGP web of > trust, or via being in validpgpkeys) need to validate? > > That does not solve the rogue maintainer issue (it is up to a packager to > ask why the number of signatures dropped...), but it does address not > needing to validate a legacy key. From a security standpoint I am fine with this approach (CC Levente for his opinion). From an implementation standpoint I will note that this complicates things a bit due to the structure of the GnuPG status file: for a valid signature, we are guaranteed to get a VALIDSIG status code containing the full key fingerprint [1], matching the information that we record in validpgpkeys. For an invalid signature, we are only guaranteed to get a BADSIG/ERRSIG containing the (long) key ID of the key [2] (because the key might not even be available locally and the signature packet alone only contains the key ID instead of the full fingerprint). So in order to check whether a bad signature was produced by a key in validpgpkeys, we need to collect the corresponding key ID and check whether there is a *partial* match with one of the fingerprints in the validpgpkeys array. I will note that it is easy to find colliding key IDs [3], so normally one should not rely on the key ID alone (even if it is the "long" 64 bit instead of the "short" 32 bit ID). In this case I think using the ID should be fine though, since we only use it for "negative" matching as an additional security check rather than relying on the key ID for "positive" matching of trusted keys: if someone manages to upload a signature file containing a bad signature with a spoofed key ID, the validation would still fail (rightfully so), while a good signature from a spoofed key ID would not be considered as valid because the fingerprint in VALIDSIG does not match. If this seems like a reasonable approach to everybody, I would be happy to implement the necessary partial comparison of the key IDs of invalid signatures in a v3 of the patch. However I would like to hear Levente's opinion first since it was he who initially brought up the concerns regarding the handling of files containing multiple signatures. Best, Jonas [1] https://github.com/gpg/gnupg/blob/master/doc/DETAILS#validsig-args [2] https://github.com/gpg/gnupg/blob/master/doc/DETAILS#badsig--long_keyid_or_fpr--username [3] https://nullprogram.com/blog/2019/07/22/
On 6/25/22 15:40, Jonas Witschel wrote: > On 2022-06-25 22:09, Allan McRae wrote: >> Somewhat offtopic - I'd argue that it is poor packaging to have more >> validpgpkeys in a PKGBUILD than the key used to verify the source for that >> particular package version. Lazily just adding more keys to a PKGBUILD and >> not removing unneeded ones in case they are needed in the future is not >> ideal. > > We should indeed be more careful in actively removing old keys from > validpgpkeys. However some upstreams have multiple active maintainers that > "arbitrarily" sign new releases, in which case keeping all of them in the array > can be helpful to convey an existing relationship of trust (without having to > go through the revision control system to figure out whether the key of a new > release has been already used in the past, or whether some other means of > establishing a chain of trust is necessary). > While I agree with old keys should be thrown out and revisited regularly whether upstream deprecated any, I disagree with only declaring the set needed for a single particular package version. If we require packagers to fiddle with the validpgpkeys array for virtually every release in a multi-maintainer project, this will weaken effective security in the long run. Requiring to re-check history and the established chain of trust for the current set of keys on each iteration will lead to a bigger window for errors and ultimately lead to fatigue related to carefully handling any changes to the validpgpkeys array. When such a security property becomes a burden to maintain, humans by nature will find a way to make it less annoying. Touching the validpgpkeys array would become the quick easygoing norm instead of a careful exception. This has also happened to pinned git sources where packagers over time simply switched back to refnames -- but this will hopefully get resolved by the immutable git sources patch. >> How about... all signatures from trusted keys (either through PGP web of >> trust, or via being in validpgpkeys) need to validate? >> >> That does not solve the rogue maintainer issue (it is up to a packager to >> ask why the number of signatures dropped...), but it does address not >> needing to validate a legacy key. > > From a security standpoint I am fine with this approach (CC Levente for his > opinion). > > From an implementation standpoint I will note that this complicates things a > bit due to the structure of the GnuPG status file: for a valid signature, we > are guaranteed to get a VALIDSIG status code containing the full key > fingerprint [1], matching the information that we record in validpgpkeys. > For an invalid signature, we are only guaranteed to get a BADSIG/ERRSIG > containing the (long) key ID of the key [2] (because the key might not even be > available locally and the signature packet alone only contains the key ID > instead of the full fingerprint). So in order to check whether a bad signature > was produced by a key in validpgpkeys, we need to collect the corresponding key > ID and check whether there is a *partial* match with one of the fingerprints in > the validpgpkeys array. > > I will note that it is easy to find colliding key IDs [3], so normally one > should not rely on the key ID alone (even if it is the "long" 64 bit instead of > the "short" 32 bit ID). In this case I think using the ID should be fine > though, since we only use it for "negative" matching as an additional security > check rather than relying on the key ID for "positive" matching of trusted > keys: if someone manages to upload a signature file containing a bad signature > with a spoofed key ID, the validation would still fail (rightfully so), while a > good signature from a spoofed key ID would not be considered as valid because > the fingerprint in VALIDSIG does not match. > > If this seems like a reasonable approach to everybody, I would be happy to > implement the necessary partial comparison of the key IDs of invalid signatures > in a v3 of the patch. However I would like to hear Levente's opinion first > since it was he who initially brought up the concerns regarding the handling of > files containing multiple signatures. > This is quite exactly what went through my mind while thinking about the proposal, hence I second what Jonas summarized. As Jonas' previous mail (before the one I'm replying to) described: A rouge maintainer would need to swap both, the tarball as well as the signature. As a simple alternative, a new version containing just a single signature could be released. Hence the signature would not need to contain any BADSIG/ERRSIG from other maintainers in both scenarios. The only real advantage that would result from the proposed changes would be if a trusted attacker is able to replace the sources but only append signatures at storage or in transit. -- which kind of sounds unlikely. I currently do not see any major disadvantages with the proposed changes besides complexity. But considering that the net benefit of this approach would be uncertain, I feel kind of mixed if that additional check would bring any effective security. Cheers, Levente
diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in index ad5bb66d..4aa14d94 100644 --- a/scripts/libmakepkg/integrity/verify_signature.sh.in +++ b/scripts/libmakepkg/integrity/verify_signature.sh.in @@ -26,6 +26,18 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'} source "$LIBRARY/util/message.sh" source "$LIBRARY/util/pkgbuild.sh" +# Returns the elements of the first array that are not found in the second +# array as the third array. All arrays are passed by name. +set_difference() { + local -n first=$1 + local -n second=$2 + local -n result=$3 + result=() + for element in "${first[@]}"; do + ! in_array "$element" "${second[@]}" && result+=("$element") + done +} + check_pgpsigs() { (( SKIPPGPCHECK )) && return 0 ! source_has_signatures && return 0 @@ -56,48 +68,53 @@ check_pgpsigs() { fi # these variables are assigned values in parse_gpg_statusfile - success=0 - status= - pubkey= - fingerprint= + success= + declare -A status + fingerprints=() trusted= parse_gpg_statusfile "$statusfile" if (( ! $success )); then printf '%s' "$(gettext "FAILED")" >&2 - case "$status" in - "missingkey") - printf ' (%s)' "$(gettext "unknown public key") $pubkey" >&2 - ;; - "revokedkey") - printf " ($(gettext "public key %s has been revoked"))" "$pubkey" >&2 - ;; - "bad") - printf ' (%s)' "$(gettext "bad signature from public key") $pubkey" >&2 - ;; - "error") - printf ' (%s)' "$(gettext "error during signature verification")" >&2 - ;; - esac + for pubkey in "${!status[@]}"; do + case "${status["$pubkey"]}" in + "missingkey") + printf ' (%s)' "$(gettext "unknown public key") $pubkey" >&2 + ;; + "revokedkey") + printf " ($(gettext "public key %s has been revoked"))" "$pubkey" >&2 + ;; + "bad") + printf ' (%s)' "$(gettext "bad signature from public key") $pubkey" >&2 + ;; + "error") + printf ' (%s)' "$(gettext "error during signature verification")" >&2 + ;; + esac + done errors=1 else + local invalid_keys + set_difference fingerprints validpgpkeys invalid_keys if (( ${#validpgpkeys[@]} == 0 && !trusted )); then - printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "$fingerprint" >&2 + printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "${fingerprints[*]}" >&2 errors=1 - elif (( ${#validpgpkeys[@]} > 0 )) && ! in_array "$fingerprint" "${validpgpkeys[@]}"; then - printf "%s (%s %s)" "$(gettext "FAILED")" "$(gettext "invalid public key")" "$fingerprint" >&2 + elif (( ${#validpgpkeys[@]} > 0 )) && (( ${#invalid_keys[@]} > 0 )); then + printf "%s (%s %s)" "$(gettext "FAILED")" "$(gettext "invalid public key")" "${invalid_keys[*]}" >&2 errors=1 else printf '%s' "$(gettext "Passed")" >&2 - case "$status" in - "expired") - printf ' (%s)' "$(gettext "WARNING:") $(gettext "the signature has expired.")" >&2 - warnings=1 - ;; - "expiredkey") - printf ' (%s)' "$(gettext "WARNING:") $(gettext "the key has expired.")" >&2 - warnings=1 - ;; - esac + for pubkey in "${!status[@]}"; do + case "${status["$pubkey"]}" in + "expired") + printf " (%s $(gettext "the signature by key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2 + warnings=1 + ;; + "expiredkey") + printf " (%s $(gettext "the key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2 + warnings=1 + ;; + esac + done fi fi printf '\n' >&2 @@ -201,59 +218,57 @@ verify_git_signature() { parse_gpg_statusfile() { local type arg1 arg6 arg10 + success=-1 + trusted=-1 while read -r _ type arg1 _ _ _ _ arg6 _ _ _ arg10 _; do case "$type" in GOODSIG) - pubkey=$arg1 - success=1 - status="good" + success=$((success!=0)) + status["$arg1"]="good" ;; EXPSIG) - pubkey=$arg1 - success=1 - status="expired" + success=$((success!=0)) + status["$arg1"]="expired" ;; EXPKEYSIG) - pubkey=$arg1 - success=1 - status="expiredkey" + success=$((success!=0)) + status["$arg1"]="expiredkey" ;; REVKEYSIG) - pubkey=$arg1 success=0 - status="revokedkey" + status["$arg1"]="revokedkey" ;; BADSIG) - pubkey=$arg1 success=0 - status="bad" + status["$arg1"]="bad" ;; ERRSIG) - pubkey=$arg1 success=0 if [[ $arg6 == 9 ]]; then - status="missingkey" + status["$arg1"]="missingkey" else - status="error" + status["$arg1"]="error" fi ;; VALIDSIG) if [[ $arg10 ]]; then # If the file was signed with a subkey, arg10 contains # the fingerprint of the primary key - fingerprint=$arg10 + fingerprints+=("$arg10") else - fingerprint=$arg1 + fingerprints+=("$arg1") fi ;; TRUST_UNDEFINED|TRUST_NEVER) trusted=0 ;; TRUST_MARGINAL|TRUST_FULLY|TRUST_ULTIMATE) - trusted=1 + trusted=$((trusted!=0)) ;; esac done < "$1" + success=$((success>0)) + trusted=$((trusted>0)) } source_has_signatures() {
PGP signature files can contain multiple signatures from different keys. In this case, verification is considered to be successful if *all* signatures are valid, compare e.g. the behaviour of the gpgv verification tool. parse_gpg_statusfile() is currently not equipped to handle the gpg output for such signature files: it assumes that the file contains only one signature and therefore overwrites the verification results of previous signatures, effectively only considering the last signature in the file. Therefore the logic needs to be refined so that $success and $trusted are only true if all signatures are valid and trusted, respectively. We also need to store the verification results per key (in order to present more specific error messages) and all the validated fingerprints (in order to check whether they are in validpgpkeys). The signatures for the source tarballs of recent GnuPG releases, e.g. version 2.2.35, are examples of such signature files. Signed-off-by: Jonas Witschel <diabonas@archlinux.org> --- .../integrity/verify_signature.sh.in | 115 ++++++++++-------- 1 file changed, 65 insertions(+), 50 deletions(-)