[pacman-dev] [PATCH v2] makepkg: Add support for verifying pgp signatures
Allan McRae
allan at archlinux.org
Mon Jul 4 10:36:57 EDT 2011
On 04/07/11 22:13, Wieland Hoffmann wrote:
> Many projects provide signature files along with the source code
> archives. It's good to check these, too, when verifying the integrity
> of source code archives.
> Not everybody is using gpg so the verification can be disabled with
> --skippgpcheck.
>
> Additionally, only a warning is displayed when the key that signed the
> source file is unknown. Expired keys and signatures aren't considered an
> error, too.
> ---
Looking good. Some general comments:
I saw that --skipinteg implies --skippgpcheck. I noticed this when I
copied a "bad" signature into my source directory and I did not update
the md5sums so used --skipinteg. I was quite surprised when the
signatures did not get checked. Should these be separated more?
I still wonder if --skippgpcheck is too long, but I can not think of a
better name. Suggestions from anyone?
> doc/makepkg.8.txt | 3 ++
> scripts/makepkg.sh.in | 72 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 74 insertions(+), 1 deletions(-)
>
> diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt
> index e11e9b3..255fbca 100644
> --- a/doc/makepkg.8.txt
> +++ b/doc/makepkg.8.txt
> @@ -169,6 +169,9 @@ Options
> in linkman:makepkg.conf[5]. If not specified in either location, the
> default key from the keyring will be used.
>
> +*\--skippgpcheck*::
> + Verify PGP signatures of the source files if provided in the build script.
> +
> *\--noconfirm*::
> (Passed to pacman) Prevent pacman from waiting for user input before
> proceeding with operations.
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 1b132a9..0b7bed6 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -57,6 +57,7 @@ FORCE=0
> INFAKEROOT=0
> GENINTEG=0
> SKIPINTEG=0
> +NOPGPSIGS=0
I'd prefer renaming this to SKIPPGPCHECK=0 to keep the name like the
flag like with SKIPINTEG.
> INSTALL=0
> NOBUILD=0
> NODEPS=0
> @@ -674,6 +675,63 @@ check_checksums() {
> fi
> }
>
> +check_pgpsigs() {
> + (( NOPGPSIGS ))&& return 0
> + (( ! ${#source[@]} ))&& return 0
We need a helper function here "source_has_signatures" that will allow
us to exit if the source array has not signatures in it. that way we
will not get the following message and then no checks. Is will also be
useful elsewhere (see below).
> + msg "$(gettext "Verifying source file signatures with gpg...")"
msg "$(gettext "Verifying source file signatures with %s...")" "gpg"
or just delete the "with gpg" part?
> +
> + local file
> + local errors=0
We should keep track of the number of non-error warnings too so a "==>
WARNING:" message could be outputed.
> + local statusfile=$(mktemp)
> +
> + for file in "${source[@]}"; do
> + local valid
> + local found=1
> +
> + file="$(get_filename "$file")"
> + if [[ $file =~ .*(sig|asc) ]]; then
Lets reduce the nested ifs here with something like:
if [[ ! $file =~ .*(sig|asc) ]]; then
continue
fi
> + echo -n " $file ... ">&2
I'd use ${file%.*} to output the name of the file being checked rather
than the signature.
> + if ! file="$(get_filepath "$file")"; then
> + echo "$(gettext "NOT FOUND")">&2
> + errors=1
> + found=0
> + fi
We should check the source file is also present.
> + if (( found )); then
Lets get rid of this and just do a "continue" instead of setting found=0
above. I'm patching the check_checksums() function to do that too...
> + if ! gpg --quiet --batch --status-file "$statusfile" --verify "$file" 2> /dev/null; then
> + if grep "NO_PUBKEY" "$statusfile"> /dev/null; then
> + echo "">&2
I think we should stick to plain output here and have a big warning at
the end (as mentioned above). See below for further comments.
> + warning "$(gettext "Unknown public key") $(awk '/NO_PUBKEY/ {print $3}' $statusfile)">&2
> + else
> + echo "$(gettext "Verification failed")">&2
> + errors=1
> + fi
> + else
> + if grep "REVKEYSIG" "$statusfile"> /dev/null; then
> + errors=1
> + echo "$(gettext "Verified, but the key that signed it has been revoked.")">&2
> + elif grep "EXPSIG" "$statusfile"> /dev/null; then
> + echo "$(gettext "Verified, but the signature is expired.")">&2
> + elif grep "EXPKEYSIG" "$statusfile"> /dev/null; then
> + echo "$(gettext "Verified, but the key that signed it is expired.")">&2
> + else
> + echo $(gettext "Verified")>&2
> + fi
> + fi
Lets unify the output with the integrity checking as much as possible.
So "FAILED" on failure, "Passed" on success.
Now for the in between cases... maybe like this?
echo "$(gettext "Warning: unknown key '%s'")" $(awk...)
echo "$(gettext "Passed")" "-" "$(gettext "Warning: key has been revoked.")"
echo "$(gettext "Passed")" "-" "$(gettext "Warning: signature has
expired.")"
echo "$(gettext "Passed")" "-" "$(gettext "Warning: key has expired.")"
How does that seem?
> + fi
> + fi
> + done
> +
> + rm -f "$statusfile"
> +
> + if (( errors )); then
> + error "$(gettext "One or more PGP signatures could not be verified!")"
> + exit 1
> + fi
if (( warnings )); then
warning "$....
fi
> +}
> +
> extract_sources() {
> msg "$(gettext "Extracting Sources...")"
> local netfile
> @@ -1478,6 +1536,14 @@ check_software() {
> fi
> fi
>
> + # gpg - source verification
> + if [[ ! NOPGPSIGS ]]; then
Use the helper function mentioned above so we only give this warning if
signatures are to be checked AND there are some to check.
> + if ! type -p gpg>/dev/null; then
> + error "$(gettext "Cannot find the %s binary required for verifying source files.")" "gpg"
> + ret=1
> + fi
> + fi
> +
> # openssl - checksum operations
> if (( ! SKIPINTEG )); then
> if ! type -p openssl>/dev/null; then
> @@ -1712,6 +1778,7 @@ usage() {
> printf "$(gettext " --key<key> Specify a key to use for %s signing instead of the default")\n" "gpg"
> printf "$(gettext " --nocheck Do not run the %s function in the %s")\n" "check()" "$BUILDSCRIPT"
> echo "$(gettext " --nosign Do not create a signature for the package")"
> + echo "$(gettext " --skippgpcheck Disable verification of source files with pgp signatures")"
> echo "$(gettext " --pkg<list> Only build listed packages from a split package")"
> printf "$(gettext " --sign Sign the resulting package with %s")\n" "gpg"
> echo "$(gettext " --skipinteg Do not fail when integrity checks are missing")"
> @@ -1749,7 +1816,7 @@ ARGLIST=("$@")
> # Parse Command Line Options.
> OPT_SHORT="AcCdefFghiLmop:rRsV"
> OPT_LONG="allsource,asroot,ignorearch,check,clean,nodeps"
> -OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver"
> +OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver,skippgpcheck"
> OPT_LONG+=",install,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
> OPT_LONG+=",repackage,skipinteg,sign,source,syncdeps,version,config:"
> # Pacman Options
> @@ -1791,6 +1858,7 @@ while true; do
> --nosign) SIGNPKG='n' ;;
> -o|--nobuild) NOBUILD=1 ;;
> -p) shift; BUILDFILE=$1 ;;
> + --skippgpcheck) NOPGPSIGS=1;;
> --pkg) shift; PKGLIST=($1) ;;
> -r|--rmdeps) RMDEPS=1 ;;
> -R|--repackage) REPKG=1 ;;
> @@ -2122,6 +2190,7 @@ if (( SOURCEONLY )); then
> if (( ! SKIPINTEG )); then
> # We can only check checksums if we have all files.
> check_checksums
> + check_pgpsigs
> else
> warning "$(gettext "Skipping integrity checks.")"
> fi
See my comment above about splitting the handling --skipinteg and
--skippgpcheck up.
> @@ -2200,6 +2269,7 @@ else
> download_sources
> if (( ! SKIPINTEG )); then
> check_checksums
> + check_pgpsigs
> else
> warning "$(gettext "Skipping integrity checks.")"
> fi
OK... that does seem a lot of comments, but they should be all quite
quick to deal with. I think this version of the patch is exactly how I
want this whole thing to be implemented, so we should be good to go
after those changes.
Allan
More information about the pacman-dev
mailing list