[pacman-dev] [PATCH 1/2] Add support for verifying pgp signatures to makepkg

[Dan McGee]

On Thu, Jun 23, 2011 at 2:36 AM, Wieland Hoffmann
<themineo at googlemail.com> wrote:

First, thanks for giving this a try.

Commit descriptions are nice to have in permanent history, but all
that stuff you wrote in the cover letter won't show up. Can you
instead include some of that right here in the patch in commit
message-style writing?

> ---
>  scripts/makepkg.sh.in |   52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 78cd4cf..cc4f152 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -516,7 +516,7 @@ download_sources() {
>        pushd "$SRCDEST" &>/dev/null
>
>        local netfile
> -       for netfile in "${source[@]}"; do
> +       for netfile in "${source[@]}" "${pgpsigs[@]}"; do
>                local file=$(get_filepath "$netfile" || true)
>                if [[ -n "$file" ]]; then
>                        msg2 "$(gettext "Found %s")" "${file##*/}"
> @@ -680,6 +680,49 @@ check_checksums() {
>        fi
>  }
>
> +check_pgpsigs() {
> +       (( ! ${#source[@]} )) && return 0
> +       (( ! ${#pgpsigs[@]})) && return 0
> +
> +       if ! type -p gpg >/dev/null; then
> +               error "$(gettext "Cannot find the gpg binary! Is gnupg installed?")"
> +               exit 1 # $E_MISSING_PROGRAM
> +       fi
Please see the check_software patch
(http://projects.archlinux.org/pacman.git/commit/?id=7468956236), this
will need to be updated to work that way instead of how we used to do
it.

> +
> +       msg "$(gettext "Validating source files with gpg...")"
> +
> +       local file
> +       local errors=0
> +
> +       for file in "${pgpsigs[@]}"; do
> +               local valid
> +               local found=1
> +
> +               file="$(get_filename "$file")"
> +               echo -n "    ${file%.sig} ... " >&2
> +
> +               if ! file="$(get_filepath "$file")"; then
What are you doing here? Assignment or comparison? Do this in two
statements rather than trying to be cute, and use an actual [[ -z ]]
block please.

> +                       echo "$(gettext "NOT FOUND")" >&2
> +                       errors=1
> +                       found=0
> +               fi
> +
> +               if (( found )); then
> +                       if ! gpg --quiet --batch --verify "$file" 2> /dev/null; then
> +                               echo "$(gettext "Verification failed")" >&2
Any need to eat stderr? If things only show up in exceptional cases,
I'd rather it come through.

> +                               errors=1
> +                       else
> +                               echo $(gettext "Verified") >&2
> +                       fi
> +               fi
> +       done
> +
> +       if (( errors )); then
> +               error "$(gettext "One or more pgp signatures could not be verified!")"
> +               exit 1
> +       fi
> +}
> +
>  extract_sources() {
>        msg "$(gettext "Extracting Sources...")"
>        local netfile
> @@ -1614,6 +1657,7 @@ usage() {
>        echo "$(gettext "  --key <key>      Specify a key to use for gpg signing instead of the default")"
>        printf "$(gettext "  --nocheck        Do not run the check() function in the %s")\n" "$BUILDSCRIPT"
>        echo "$(gettext "  --nosign         Do not create a signature for the package")"
> +       echo "$(gettext "  --pgp            Enable verification of source files with pgp signatures")"
I'm not real keen on this option as it isn't clear whether it deals
with signing or verification. I think --verify would be better, but
I'll leave that up to Allan.

>        echo "$(gettext "  --pkg <list>     Only build listed packages from a split package")"
>        echo "$(gettext "  --sign           Sign the resulting package with gpg")"
>        echo "$(gettext "  --skipinteg      Do not fail when integrity checks are missing")"
> @@ -1651,7 +1695,7 @@ ARGLIST=("$@")
>  # Parse Command Line Options.
>  OPT_SHORT="AcCdefFghiLmop:rRsV"
>  OPT_LONG="allsource,asroot,ignorearch,check,clean,cleancache,nodeps"
> -OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver"
> +OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver,pgp"
>  OPT_LONG+=",install,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
>  OPT_LONG+=",repackage,skipinteg,sign,source,syncdeps,version,config:"
>  # Pacman Options
> @@ -1694,6 +1738,7 @@ while true; do
>                --nosign)         SIGNPKG='n' ;;
>                -o|--nobuild)     NOBUILD=1 ;;
>                -p)               shift; BUILDFILE=$1 ;;
> +               --pgp)            PGPSIGS=1;;
>                --pkg)            shift; PKGLIST=($1) ;;
>                -r|--rmdeps)      RMDEPS=1 ;;
>                -R|--repackage)   REPKG=1 ;;
> @@ -2129,6 +2174,9 @@ else
>        download_sources
>        if (( ! SKIPINTEG )); then
>                check_checksums
> +               if (( PGPSIGS )); then
> +                       check_pgpsigs
> +               fi
This check should probably match how we do it elsewhere; e.g.,
check_signature() the first two lines. I'd move it inside
check_pgpsigs itself. Also declare it upfront where we set the rest of
these.

>        else
>                warning "$(gettext "Skipping integrity checks.")"
>        fi
> --
> 1.7.5.4