Bug#821051: git branch with code signing script

[Julien Cristau]

On Tue, Oct  4, 2016 at 20:30:36 +0200, Jakub Wilk wrote:

> I don't know anything about dak, PKCS#$N, or EFI; but Helen asked me for a
> review, so here it goes:
> 
Thanks for doing this.

> > +expect "Enter Password *:" {send $pin} timeout {exit 1}
> > +expect "Enter passphrase *:" {send $pin} timeout {exit 1}
> 
> This is weird. Why two different prompts exist?
> 
IIRC these are
https://github.com/rhinstaller/pesign/blob/master/src/password.c#L270
and
https://github.com/rhinstaller/pesign/blob/master/src/password.c#L320

Cheers,
Julien

Bug#821051: git branch with code signing script

[Jakub Wilk]

I don't know anything about dak, PKCS#$N, or EFI; but Helen asked me for a 
review, so here it goes:


* Helen Koike , 2016-09-28, 14:39:

+chmod a+r $IN_TARBALL


No, if the input tarball isn't world-writable, then it was certainly made so 
for a reason, and you shouldn't expose it to all local users. (Even if it 
didn't hurt from security perspective, this script is a wrong place to change 
permissions of this file anyway.)


Here, and in other places, variables are not double-quoted. I don't think it 
makes a difference in this case, but the style in inconsistent with the rest of 
the code, and ""-less code in unnecessarily difficult to review.



+sudo -u codesign ${0%/*}/byhand-code-sign-user $IN_TARBALL $OUT_TARBALL 
"$configdir/byhand-code-sign.conf"


The codesign user wouldn't have permissions to create the output tarball, I 
guess...



+sudo chown dak $OUT_TARBALL


Hopefully the dak user is not permitted to elevate to root!

Instead of playing with file permissions, it would be much better (and simpler) 
for byhand-code-sign-user to read the input file from stdin and spew the 
tarball with sigs on stdout.



+   CERT_DIR="$(mktemp -td byhand-code-sign-cert.XX)"
+   chmod 700 "$CERT_DIR"


mktemp(1) calls mkdir(..., 0700), so the chmod is not necessary. (And if mktemp 
didn't do the right thing, then there would be race window between mktemp and 
chmod, in which the directory could be opened by other users.)



+   ${0%/*}/byhand-code-sign-user-exp $IN_DIR/$filename 
$OUT_DIR/$filename.sig $pkcs11_pin_value ${PESIGN_PARAMS[@]}


Don't put secrets, such as $pkcs11_pin_value, on the command line. Command 
lines are visible to all local users via /proc/$pid/cmdline.



+chmod -R a+rX "$OUT_DIR"


This unnecessarily exposes the files to local users. (Though I guess it doesn't 
matter in this case.) You can ask tar to normalize permissions for you: 
--mode=a+rX. You may also want to normalize ownership: --owner=root 
--group=root.



+expect "Enter Password *:" {send $pin} timeout {exit 1}
+expect "Enter passphrase *:" {send $pin} timeout {exit 1}


This is weird. Why two different prompts exist?

--
Jakub Wilk

Bug#821051: git branch with code signing script

[Helen Koike]

Hi,

On Sun, 25 Sep 2016 01:06:06 +0100 Ben Hutchings  wrote:
> I've pushed all my changes to a git repo at:
> https://git.decadent.org.uk/git/dak.git
>
> Ben.
>
> --
> Ben Hutchings
> No political challenge can be met by shopping. - George Monbiot

I modified this code and also got some code from Julien Cristau to sign the 
packages as another user, please, see patch below and let me know your feedback.
The following patch applies on top of https://git.decadent.org.uk/git/dak.git
I didn't teste with a real token because I don't have one, could someone check 
this please?

Thank you
Helen Koike

diff --git a/scripts/debian/byhand-code-sign b/scripts/debian/byhand-code-sign
index fbd6855..6b48d26 100755
--- a/scripts/debian/byhand-code-sign
+++ b/scripts/debian/byhand-code-sign
@@ -20,8 +20,6 @@ error() {
exit 1
 }
 
-export OPENSSL_CONF=/dev/null
-
 # Read dak configuration for security or main archive.
 # Also determine subdirectory for the suite.
 case "$0" in
@@ -39,116 +37,12 @@ case "$0" in
 esac
 . "$configdir/vars"
 
-# Read and trivially validate our configuration
-. "$configdir/byhand-code-sign.conf"
-for var in EFI_BINARY_PRIVKEY EFI_BINARY_CERT \
-  LINUX_SIGNFILE LINUX_MODULE_PRIVKEY LINUX_MODULE_CERT; do
-   test -v $var || error "$var is not defined in configuration"
-   test -n "${!var}" || error "$var is empty in configuration"
-done
-
 TARGET="$ftpdir/dists/$suitedir/main/code-sign/"
 OUT_TARBALL="$TARGET/${IN_TARBALL##*/}"
 OUT_TARBALL="${OUT_TARBALL%.tar.xz}_sigs.tar.xz"
 
-# Check that this source/arch/version hasn't already been signed
-if [ -e "$OUT_TARBALL" ]; then
-   error "Signature tarball already exists: $OUT_TARBALL"
-fi
-
-# If we fail somewhere, cleanup the temporary directories
-IN_DIR=
-OUT_DIR=
-CERT_DIR=
-cleanup() {
-   for dir in "$IN_DIR" "$OUT_DIR" "$CERT_DIR"; do
-   test -z "$dir" || rm -rf "$dir"
-   done
-}
-trap cleanup EXIT
-
-# Extract the data into the input directory
-IN_DIR="$(mktemp -td byhand-code-sign-in.XX)"
-tar xaf "$IN_TARBALL" --directory="$IN_DIR"
-
-case "$EFI_BINARY_PRIVKEY" in
-pkcs11:*)
-   # Translate from OpenSSL PKCS#11 enigne syntax to pesign parameters
-   # See: 
https://sources.debian.net/src/engine-pkcs11/0.2.2-1/src/engine_pkcs11.c
-   pkcs11_pin_value=
-   old_IFS="$IFS"
-   IFS=';'
-   for kv in ${EFI_BINARY_PRIVKEY#pkcs11:}; do
-   case "$kv" in
-   token=*)
-   pkcs11_token="${kv#*=}"
-   ;;
-   object=*)
-   pkcs11_object="${kv#*=}"
-   ;;
-   pin-value=*)
-   pkcs11_pin_value="${kv#*=}"
-   ;;
-   esac
-   done
-   IFS="$old_IFS"
-   unset old_IFS
-   # TODO: unlock it
-   PESIGN_PARAMS=(-t "$pkcs11_token" -c "$pkcs11_object")
-   ;;
-*)
-   # Create certificate store for pesign
-   CERT_DIR="$(mktemp -td byhand-code-sign-cert.XX)"
-   chmod 700 "$CERT_DIR"
-   mkdir "$CERT_DIR/store"
-   certutil -N --empty-password -d "$CERT_DIR/store"
-   openssl pkcs12 -export \
-   -inkey "$EFI_BINARY_PRIVKEY" -in "$EFI_BINARY_CERT" \
-   -out "$CERT_DIR/efi-image.p12" -passout pass: \
-   -name efi-image
-   pk12util -i "$CERT_DIR/efi-image.p12" -d "$CERT_DIR/store" -K '' -W ''
-   PESIGN_PARAMS=(-n "$CERT_DIR/store" -c efi-image)
-   ;;
-esac
-
-# Create hierarchy of detached signatures in parallel to the uploaded files
-OUT_DIR="$(mktemp -td byhand-code-sign-out.XX)"
-while read filename; do
-   mkdir -p "$OUT_DIR/${filename%/*}"
-   case "${filename##*/}" in
-   *.efi | vmlinuz-*)
-   pesign -i "$IN_DIR/$filename" \
-  --export-signature "$OUT_DIR/$filename.sig" --sign \
-  -d sha256 "${PESIGN_PARAMS[@]}"
-   ;;
-   *.ko)
-   "$LINUX_SIGNFILE" -d sha256 "$LINUX_MODULE_PRIVKEY" \
-   "$LINUX_MODULE_CERT" "$IN_DIR/$filename"
-   mv "$IN_DIR/$filename.p7s" "$OUT_DIR/$filename.sig"
-   ;;
-   *)
-   echo >&2 "W: Not signing unrecognised file: $filename"
-   continue
-   ;;
-   esac
-   if [ ${#filename} -gt 60 ]; then
-   filename_trunc="...${filename:$((${#filename} - 57)):57}"
-   else
-   filename_trunc="$filename"
-   fi
-   printf 'I: Signed %-60s\r' "$filename_trunc"
-done < <(find "$IN_DIR" -type f -printf '%P\n')
-
-# Clear last progress message
-printf '%-70s\r' ''
-
-# Build tarball of signatures
-chmod -R a+rX "$OUT_DIR"
-mkdir -p "$TARGET"
-tar caf "$OUT_TARBALL" --directory="$OUT_DIR" .
-echo "I: Created $OUT_TARBALL"
-
-trap - EXIT
-cleanup
+chmod a+r $IN_TARBALL
+sudo -u codesign 

Bug#821051: git branch with code signing script

[Ben Hutchings]

I've pushed all my changes to a git repo at:
https://git.decadent.org.uk/git/dak.git

Ben.

-- 
Ben Hutchings
No political challenge can be met by shopping. - George Monbiot

signature.asc
Description: This is a digitally signed message part