Closed Bug 409479 Opened 17 years ago Closed 15 years ago

create script which wraps existing signing steps on signing machine

Categories

(Release Engineering :: General, defect, P2)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joduinn, Assigned: bhearsum)

References

Details

Attachments

(2 files, 7 obsolete files)

The signing step currently does:
- GPG signing for linux
- GPG signing for mac (bug#400296 is about adding signing to Mac builds)
- Authenticode and GPG signing for win32

Currently, we do all this signing manually on the signing machine by following a set of written instructions on wiki.

Can we create a script, which basically wraps what we do manually now? While this might seem sub-optimal, it would:
- reduce the risk of human error
- speed up the signing process
- be a relatively simple incremental change
- simplify future automation of Build Signing.
- allow us to keep doing signing the "traditional" way, in case we find a problem at the last minute, during the signing phase of a release.

For security reasons, we'd still insert the USBkey, and password, manually.
Priority: -- → P3
Blocks: 412006
Triaged to Future.
Component: Release Engineering → Release Engineering: Future
QA Contact: build → release
I'll be doing this in Q1.
Status: NEW → ASSIGNED
Component: Release Engineering: Future → Release Engineering
Reassigning based on comment #2.
Assignee: nobody → bhearsum
Status: ASSIGNED → NEW
Blocks: 433930
Priority: P3 → P2
Attached file one possible approach (obsolete) —
I looked at doing this as a Perl script or bash, too, but I think this will work best since we get halt-on-failure for free with a Makefile.

This is untested as of yet, not ready for review.
Attached file fixed up a little bit (obsolete) —
Attachment #365926 - Attachment is obsolete: true
Attached file v3 (obsolete) —
Attachment #365930 - Attachment is obsolete: true
Attached file v4 (obsolete) —
Attachment #365939 - Attachment is obsolete: true
Attached file fully tested version (obsolete) —
Alright, after a few iterations this is a mostly complete version. With this, the signing instructions should be cut down to the following:
export PRODUCT=firefox
export VERSION=3.1b3
export BUILD=1
export TAG=FIREFOX_3_1b3_RELEASE
export REPO=releases/mozilla-1.9.1
mkdir -p ~/signing-work/${PRODUCT}-${VERSION}
cd ~/signing-work/${PRODUCT}-${VERSION}
cp ~/something/Makefile .
make all
<wait>
make upload


One thing I'm not entirely sure about yet is whether I like sign-files running as part of this. It's sortof unfortunate because it hangs the process until the passphrase is entered. On the other hand, it's still faster than doing it by hand separately. Same thing goes for verify-signatures.

We might be able to speed up the sign-files part by creating a passphraseless version of the key...but that's probably a bad idea.

I did a test run of this with en-US + 1 locale and it worked great. I specifically the tested the verify-{win32,asc} targets by mucking around in signed-build1/ and they properly printed and erred.

The only thing I haven't actually tested here is the real 'upload' target, because the staging signing machine doesn't have access to the build network, and I decided against pushing to stage.m.o ;).
Attachment #365947 - Attachment is obsolete: true
Attachment #365980 - Flags: review?(nthomas)
Comment on attachment 365980 [details]
fully tested version

> One thing I'm not entirely sure about yet is whether I like sign-files running
> as part of this. It's sortof unfortunate because it hangs the process until the
> passphrase is entered. On the other hand, it's still faster than doing it by
> hand separately. Same thing goes for verify-signatures.

We could send an email when it hits sign-files, so that we don't have to pay attention until that point. Otherwise we'd be heading into madcap schemes where (eg) sign-files runs early to capture the passphrase, then sleeps until it gets a specific signal.

>STAGE_USERNAME ?= ffxbld
>SSH_KEY        ?= $(HOME)/.ssh/ffxbld_dsa
>WORKDIR         = $(HOME)/signing-work/$(PRODUCT)-$(VERSION)
>PRODUCT        ?= firefox

Should do these last two in the opposite order, and ensure version is defined. Maybe check on BUILD, REPO, TAG being defined as well.

>MAR				= $(WORKDIR)/mar.exe
>SEVENZIP		= $(WORKDIR)/7-Zip/7z.exe
>UPX_BIN			= $(WORKDIR)/upx/upx.exe

Some sort of tabs vs spaces formatting glitch going on here.

>	rsync -av --exclude '*unsigned*' unsigned-build$(BUILD)/ signed-build$(BUILD)/

Aside: I noticed with 3.1b3 that this step chews up a ton of disk space and is quite slow, worse than before because the update files are there. Maybe we can just symlink signed-buildN/linux-i686 back to unsigned-buildN/linux-i686 (and the other 3 cases) to avoid this. And/or not bother to sync down linux and mac update files. Not something we have to fix here though.

>verify-signatures:
>	cd $(WORKDIR)/signed-build$(BUILD)/win32 && verify-signatures

Turns out you can give a /q argument to chktrust (which is what verify-signatures is calling) to avoid the dialog. Exit status is the usual unixy 0 or 1 for success and failure. What do you think ? I checked the signed and unsigned cases but we should test round a bit more before trusting it.

>verify-win32: DIFF_FILES=$(strip $(shell diff -rq $(WORKDIR)/unsigned-build$(BUILD)/unsigned/ \
>                           signed-build$(BUILD)/ | grep -E '(\.exe|\.mar)\ ' | wc -l))
>verify-win32: TOTAL_FILES=$(strip $(shell find $(WORKDIR)/unsigned-build$(BUILD)/unsigned/ \
>                            -name '*.exe' -or -name '*.mar' | wc -l))

Curious why these have a separate target. Also, the tests didn't fail for me if the directory doesn't exist. Obviously we're gonna die earlier than this if that's the case, but something isn't paying attention to exit status here. Checking they're greater than 0 is one way to work around this. Same for verify-asc. 

r- for the small number of issues here. From the sounds of it you've checked the case where signing works without errors, and around the verification targets. Have you made sure that the other scripts return sensible exit status on errors ? Eg passphrases not entered or wrong. I'm a bit wary of automating to a position where errors scroll off the screen quickly.
Attachment #365980 - Flags: review?(nthomas) → review-
(In reply to comment #9)
> (From update of attachment 365980 [details])
> > One thing I'm not entirely sure about yet is whether I like sign-files running
> > as part of this. It's sortof unfortunate because it hangs the process until the
> > passphrase is entered. On the other hand, it's still faster than doing it by
> > hand separately. Same thing goes for verify-signatures.
> 
> We could send an email when it hits sign-files, so that we don't have to pay
> attention until that point. Otherwise we'd be heading into madcap schemes where
> (eg) sign-files runs early to capture the passphrase, then sleeps until it gets
> a specific signal.

Yeah, that's a good idea. How do you feel about e-mailing being sent at other points, too? Namely, right before and after sign-release.pl. Obviously the latter wouldn't get sent if sign-release.pl fails, which might help raise an alarm ("hey! why didn't i get a sign-release.pl completion e-mail").

> 
> >STAGE_USERNAME ?= ffxbld
> >SSH_KEY        ?= $(HOME)/.ssh/ffxbld_dsa
> >WORKDIR         = $(HOME)/signing-work/$(PRODUCT)-$(VERSION)
> >PRODUCT        ?= firefox
> 
> Should do these last two in the opposite order, and ensure version is defined.
> Maybe check on BUILD, REPO, TAG being defined as well.
> 

Yeah, good point.

> Some sort of tabs vs spaces formatting glitch going on here.
> 

Fixed.

> Aside: I noticed with 3.1b3 that this step chews up a ton of disk space and is
> quite slow, worse than before because the update files are there. Maybe we can
> just symlink signed-buildN/linux-i686 back to unsigned-buildN/linux-i686 (and
> the other 3 cases) to avoid this. And/or not bother to sync down linux and mac
> update files. Not something we have to fix here though.
> 

I'll file a follow-up on this. Mostly because of time constraints I don't want to do it as part of this.

> >verify-signatures:
> >	cd $(WORKDIR)/signed-build$(BUILD)/win32 && verify-signatures
> 
> Turns out you can give a /q argument to chktrust (which is what
> verify-signatures is calling) to avoid the dialog. Exit status is the usual
> unixy 0 or 1 for success and failure. What do you think ? I checked the signed
> and unsigned cases but we should test round a bit more before trusting it.
> 

This looks promising. It's probably best/easiest to just modify verify-signatures to do this. It seems chktrust doesn't work if you're not in the directory which contains the file you're testing. I did some testing of chktrust with and without -q on various unsigned and signed files. Are there other specific tests you want to do first?

> >verify-win32: DIFF_FILES=$(strip $(shell diff -rq $(WORKDIR)/unsigned-build$(BUILD)/unsigned/ \
> >                           signed-build$(BUILD)/ | grep -E '(\.exe|\.mar)\ ' | wc -l))
> >verify-win32: TOTAL_FILES=$(strip $(shell find $(WORKDIR)/unsigned-build$(BUILD)/unsigned/ \
> >                            -name '*.exe' -or -name '*.mar' | wc -l))
> 
> Curious why these have a separate target.

Are you asking why they aren't part of verify-signatures? If so...
Only because they are sortof different things. I'm happy to do it either way, though. Let me know if you want me to make that change.

> Also, the tests didn't fail for me if
> the directory doesn't exist. Obviously we're gonna die earlier than this if
> that's the case, but something isn't paying attention to exit status here.
> Checking they're greater than 0 is one way to work around this. Same for
> verify-asc. 

Good catch, I'll fix that.

> 
> r- for the small number of issues here. From the sounds of it you've checked
> the case where signing works without errors, and around the verification
> targets. Have you made sure that the other scripts return sensible exit status
> on errors ? Eg passphrases not entered or wrong. I'm a bit wary of automating
> to a position where errors scroll off the screen quickly.

I'll check that and do some more testing of failure cases.
Status: NEW → ASSIGNED
Most of the changes here are additive, here's a list:
* Big header comment with example usage and other details
* Error out when VERSION, BUILD, REPO, or TAG are not set
* Send e-mail with blat at a few points throughout
* Fix order of variables to make sure WORKDIR actually works
* Properly pipe STDERR and STDOUT to the signing log
** Note that 'tee' was removed here because with it, $? is tee's exit code, which doesn't work so well in a Makefile
* verify-win32/verify-asc now makes sure there's at least one file in TOTAL_FILES.
** I couldn't find a way to do a -gt comparison in Makefile syntax so I switched all of those comparisons to 'test', for consistency.

I've got a patch incoming that fixes sign-release, sign-release.pl, sign-files, and verify-signatures to properly error out. Standby for that.
Attachment #365980 - Attachment is obsolete: true
Attachment #368300 - Flags: review?(nthomas)
sign-release:
* Error out if there are no files to be processed

verify-signatures:
* Error out if there are no files to be processed
* Use chktrust /q to silently test. Note that in staging this will *always* fail, presumably because we use a self signed cert there. Worked fine in all the tests I did on real builds.
* Print out a summary of failures at the end, return non-zero exit code

sign-files:
* Error out if there are no files to be processed
* Use die() instead of printf() for GPG passphrase and other errors. This properly halted the Makefile when the wrong passphrase was used.

sign-release.pl:
This file actually has a lot of great error handling in it already...it just didn't properly bubble up. main() returns the exit code of whatever subroutine it's calling, which return non-zero on errors. The only changes I had to make was to have ProcessWin32{Build,Directory} return 0 when they successfully run to completion, and to use 'exit main()' so we return with whatever main() gives us.

I did a minimal run (en-US + 4 locales) with this patch + the latest Makefile which worked great. I tested some error conditions too: signed-build1 not existing/being empty, bad passphrase for GPG, bad passphrase for signcodepwd.exe, empty unsigned-build1/{unsigned/} and all of the cases were handled correctly.

I've got a full testrun of 3.1b1 going right now that is going OK so far.
Attachment #368302 - Flags: review?(nthomas)
Ok, my testrun "passed". There were a couple snags, but they don't represent real problems:
* 'sign' target failed because win32_info.txt didn't exist - this is because I was testing against 3.1b1, in which _info.txt files were created manually in the candidates dir and win32_info.txt was never in unsigned/
* verify-signatures failed because of the use of the staging key

Other than that everything ran great.
Comment on attachment 368302 [details] [diff] [review]
fix signing scripts to error properly

>Index: sign-files

Oh my, the indentation in this file suxors. :-S

>Index: sign-release.pl
>@@ -764,7 +764,7 @@ ProcessWin32Build()
>    if (!$rv) {
>       die "ProcessWin32Build(): ProcessWin32Zip() failed at some point. :-(\n";
>    }  
>-   return 1;
>+   return 0;
> }
> 
>@@ -1182,10 +1182,10 @@ ProcessWin32Directory()
>       ResetActiveChecksumHash();
>    }
> 
>-   return 1;
>+   return 0;
> }
> 
> autoflush STDOUT 1;
> autoflush STDERR 1;
> 
>-main();
>+exit main();

Most functions in this file still return 1 on success, except for these two. Perhaps it would be better to keep returning 1 inside the script, and convert to exit codes at the very end ? A simple negation would do the trick I think. r+ with that fixed up.
Attachment #368302 - Flags: review?(nthomas) → review+
Comment on attachment 368300 [details]
signing Makefile, with good header comment and better error detection

Looks good, r+ with a few comments below.

># When you're satisfied with the builds and ready to upload them you can run:
>#  make upload
># With all of the same variable definitions which will rsync the signed builds

Nits: More indentation on "make upload", s/^With/with/

>verify-signatures:
>	cd $(WORKDIR)/signed-build$(BUILD)/win32 && verify-signatures
>
>verify-win32: DIFF_FILES=$(strip $(shell diff -rq $(WORKDIR)/unsigned-build$(BUILD)/unsigned/ \
>                           signed-build$(BUILD)/ | grep -E '(\.exe|\.mar)\ ' | wc -l))

These names have historical significance but I'm finding it more confusing than useful these days. Could you stick in some comments, eg
 # Verify installer signing is valid and can be traced back to a valid root cert
and
 # Verify that we modified every unsigned installer and complete mar

Which makes me wonder if we could do more complete checks by running chktrust against all the dll's and exe's before recreating an exe/mar. I guess the error handling in sign-release.pl ensures this indirectly.
Attachment #368300 - Flags: review?(nthomas) → review+
Forgot to say "nice one" on all the messaging by mail, that'll help a bunch with waiting around.

Ideas for followups (and a stab at what they'd require)
* handle Tb2.0.0.x/Fx 3.0.x signing too (new toplevel target, different d/l, different call to sign-release.pl)
* handle partner build signing (new toplevel target, different download/upload logic, just installer and gpg signing)
* use sign-release, verify-signatures, sign-files from our checkout rather than ~/bin (like we do for sign-release.pl)
(In reply to comment #14)
> (From update of attachment 368302 [details] [diff] [review])
> >Index: sign-files
> 
> Oh my, the indentation in this file suxors. :-S
> 
> >Index: sign-release.pl
> >@@ -764,7 +764,7 @@ ProcessWin32Build()
> >    if (!$rv) {
> >       die "ProcessWin32Build(): ProcessWin32Zip() failed at some point. :-(\n";
> >    }  
> >-   return 1;
> >+   return 0;
> > }
> > 
> >@@ -1182,10 +1182,10 @@ ProcessWin32Directory()
> >       ResetActiveChecksumHash();
> >    }
> > 
> >-   return 1;
> >+   return 0;
> > }
> > 
> > autoflush STDOUT 1;
> > autoflush STDERR 1;
> > 
> >-main();
> >+exit main();
> 
> Most functions in this file still return 1 on success, except for these two.
> Perhaps it would be better to keep returning 1 inside the script, and convert
> to exit codes at the very end ? A simple negation would do the trick I think.
> r+ with that fixed up.

urgh. Thank you for catching that. I'll make that change upon checkin.
Checking in sign-files;
/mofo/release/signing/tools/sign-files,v  <--  sign-files
new revision: 1.3; previous revision: 1.2
done
Checking in sign-release;
/mofo/release/signing/tools/sign-release,v  <--  sign-release
new revision: 1.5; previous revision: 1.4
done
Checking in sign-release.pl;
/mofo/release/signing/tools/sign-release.pl,v  <--  sign-release.pl
new revision: 1.33; previous revision: 1.32
done
Checking in verify-signatures;
/mofo/release/signing/tools/verify-signatures,v  <--  verify-signatures
new revision: 1.2; previous revision: 1.1
done


Nick, this patch should have your comments addressed.
Attachment #368302 - Attachment is obsolete: true
Attachment #369068 - Flags: checked‑in+
Nick, I fixed the top comment and added comments to verify-*. Going to go ahead and land this.

build/tools: changeset:   250:20095d63ab91
Attachment #368300 - Attachment is obsolete: true
Attachment #369070 - Flags: checked‑in+
I just updated https://intranet.mozilla.org/Build:CombinedSigning with the new instructions. Nick, are you planning to do another testrun before 3.1b4? It could be a good time to give this another run-through...

I'm pretty sure this bug can be declared FIXED now, though.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I'll do another test as part of bug 481469. Thanks for working on this Ben.
Almost forgot to setup keymaster.

To install blat:
as administrator:
cvs -d:ext:cltbld@cvs.mozilla.org:/mofo co ref-platforms/win32/blat-2.50
cd ref-platforms/win32/blat-2.50
unzip Blat250.zip
cd Blat250/full
cp * /usr/local/bin
cd /usr/local/bin
chmod +x *.exe
blat -install mail.mozilla.com keymaster@office.mozilla.org 5
as cltbld:
blat -to bhearsum@mozilla.com -subject 'TEST' -body 'TEST123'

Install Mercurial, checkout build/tools:
as administrator:
download Mercurial from selenic.com/mercurial (version 1.2.1)
as cltbld in cygwin:
cd ~/
hg clone https://hg.mozilla.org/build/tools hg-tools
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: