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