make_dev_firmware: new script to change firmware to dev key

make_dev_firmware: new script to change firmware to dev key

The make_dev_firmware.sh is made for devinstall shim to
change firmware rootkey/HWID/BMPFV smoothly.

 - HWID will be changed to "$ORIGINAL_FWID DEV" (no change if already postfixed
   with DEV)
 - rootkey/recoverykey will be changed by keyset from --keys
 - FVMAIN/FVMAINB will be resigned by keyset from --keys
 - BMPFV will be changed to anything assigned by --bmpfv

 If --from and --to are omitted, the system firmware will be changed.
 A new ebuild is be created to put all resources (bmpfv and keyset) into devinstall shim (ref: http://codereview.chromium.org/3776003)

BUG=chrome-os-partner:1276
TEST=sudo ./make_dev_firmware.sh --from input_bios.bin --to output_bios.in \
 --keys ../../tests/devkeys --bmpfv some_bmpfv.bin
HWID is changed from "XXX MARIO EVT DDDD" to "XXX MARIO EVT DDDD DEV".
System can then boot a USB signed with devkey without developer mode.

Change-Id: Id80126495dcbf4d993a4372af645580cd4b60ca6

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=3bdfc46

[Hung-Te, 2010-10-14 09:05:31]

randall: please confirm if the behavior is compliant to what you expected
gauravsh: please review the script itself
wfrichar: FYI

[Bill Richardson, 2010-10-14 16:52:55]

Some comments.

http://codereview.chromium.org/3822002/diff/11001/12002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/11001/12002#newcode98
scripts/image_signing/make_dev_firmware.sh:98: # should we also consider {GUID}
as DEV names?
# Answer: No. Officially-signed firmware should always have an officially
approved HWID.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode168
scripts/image_signing/make_dev_firmware.sh:168: --rootkey="$old_rootkey_file" \
Why are you extracting the old rootkey? You don't use it anywhere, and the
original BIOS image in $FLAGS_backup_dir contains everything you need.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode220
scripts/image_signing/make_dev_firmware.sh:220: 
I think we want an option to re-sign the kernel image(s) too, don't we?
Otherwise, we'll be forced into recovery as soon as we reboot in normal mode.

[v.b, 2010-10-14 17:28:18]

Hung-Te, looks good, just a few comments to your discretion.

Also, it would be great to extend the test spec to demonstrate HW id before and
after, verifying booting prod image before and dev image after, etc. or at the
very least running vbutil_firmware --verify on the image before and after.

http://codereview.chromium.org/3822002/diff/11001/12002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/11001/12002#newcode49
scripts/image_signing/make_dev_firmware.sh:49: # Returns if we're running in
debug mode
s/Returns/Returns true/

http://codereview.chromium.org/3822002/diff/11001/12002#newcode113
scripts/image_signing/make_dev_firmware.sh:113: echo "ERROR: Cannot find
required file: $file"
this would be more user friendly if it was reporting all missing files at once,
not one at a time. Just a thought.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode129
scripts/image_signing/make_dev_firmware.sh:129: local is_from_live=0
do you really need this variable - what's wrong with using ${FLAGS_from}?

http://codereview.chromium.org/3822002/diff/11001/12002#newcode170
scripts/image_signing/make_dev_firmware.sh:170: grep hardware_id |
grep+sed could be replaced by
awk '{if (sub("^hardware_id: ","")) print}'

[Hung-Te, 2010-10-14 17:47:06]

Thanks for your review. Here's the revised version.

http://codereview.chromium.org/3822002/diff/11001/12002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/11001/12002#newcode49
scripts/image_signing/make_dev_firmware.sh:49: # Returns if we're running in
debug mode
On 2010/10/14 17:28:18, v.b wrote:
> s/Returns/Returns true/

Done.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode98
scripts/image_signing/make_dev_firmware.sh:98: # should we also consider {GUID}
as DEV names?
But we already have some people flashed their device with those "default" IDs.
They may feel weird when seeing a "{XXX} DEV".

Anyway I'm OK to simply ignore that.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode113
scripts/image_signing/make_dev_firmware.sh:113: echo "ERROR: Cannot find
required file: $file"
On 2010/10/14 17:28:18, v.b wrote:
> this would be more user friendly if it was reporting all missing files at
once,
> not one at a time. Just a thought.

Done.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode129
scripts/image_signing/make_dev_firmware.sh:129: local is_from_live=0
First, right now only backup check this, but if we have more in the future,
is_from_live would be more readable.
Second, we may have some test/command to trigger "work like having live source".

I'd prefer to keep this if you're OK with that.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode168
scripts/image_signing/make_dev_firmware.sh:168: --rootkey="$old_rootkey_file" \
Because I wanted to do a compare of rootkey/VBOOTA/VBOOTB (see the TODO below)
and quick abort / prompt user if he's already using dev keys.

After checking again current flow, I think this is not necessary. I can move the
test to the last step. Thanks.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode170
scripts/image_signing/make_dev_firmware.sh:170: grep hardware_id |
I prefer to not use awk because we have different versions of awk on
host/netbook (gawk v.s. mawk), and it's painful to test/maintain the
compatibility of awk.

http://codereview.chromium.org/3822002/diff/11001/12002#newcode220
scripts/image_signing/make_dev_firmware.sh:220: 
Not sure if we really need that (resign kernel on SSD), because that would
changed the OS image.

The command is supposed to be executed in developer mode (boot by devinstall
shim), so you can still boot SSD in dev mode, or boot with USB and install a new
OS without dev mode.

But I agree an option that also updates SSD may be helpful to some use-cases.
Maybe put that in another CL? (Or do yo think that should be in the first
release of this tool?)

[Bill Richardson, 2010-10-14 18:11:19]

LGTM, with one comment.

http://codereview.chromium.org/3822002/diff/11001/12002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/11001/12002#newcode220
scripts/image_signing/make_dev_firmware.sh:220: 
We can make this a separate CL, if we decide it's needed. I bring it up because
currently we can only have one valid image after transitioning between dev and
normal mode (bug 7706), so if we're trying to debug an EVT image we'd have to
reinstall the same kernel manually after rekeying but if we rekey it now we
wouldn't.

[v.b, 2010-10-14 18:14:20]

LGTM

thank you for addressing my concerns.

On 2010/10/14 17:47:06, Hung-Te wrote:
> Thanks for your review. Here's the revised version.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002
> File scripts/image_signing/make_dev_firmware.sh (right):
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode49
> scripts/image_signing/make_dev_firmware.sh:49: # Returns if we're running in
> debug mode
> On 2010/10/14 17:28:18, v.b wrote:
> > s/Returns/Returns true/
> 
> Done.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode98
> scripts/image_signing/make_dev_firmware.sh:98: # should we also consider
{GUID}
> as DEV names?
> But we already have some people flashed their device with those "default" IDs.
> They may feel weird when seeing a "{XXX} DEV".
> 
> Anyway I'm OK to simply ignore that.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode113
> scripts/image_signing/make_dev_firmware.sh:113: echo "ERROR: Cannot find
> required file: $file"
> On 2010/10/14 17:28:18, v.b wrote:
> > this would be more user friendly if it was reporting all missing files at
> once,
> > not one at a time. Just a thought.
> 
> Done.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode129
> scripts/image_signing/make_dev_firmware.sh:129: local is_from_live=0
> First, right now only backup check this, but if we have more in the future,
> is_from_live would be more readable.
> Second, we may have some test/command to trigger "work like having live
source".
> 
> I'd prefer to keep this if you're OK with that.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode168
> scripts/image_signing/make_dev_firmware.sh:168: --rootkey="$old_rootkey_file"
\
> Because I wanted to do a compare of rootkey/VBOOTA/VBOOTB (see the TODO below)
> and quick abort / prompt user if he's already using dev keys.
> 
> After checking again current flow, I think this is not necessary. I can move
the
> test to the last step. Thanks.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode170
> scripts/image_signing/make_dev_firmware.sh:170: grep hardware_id |
> I prefer to not use awk because we have different versions of awk on
> host/netbook (gawk v.s. mawk), and it's painful to test/maintain the
> compatibility of awk.
> 
> http://codereview.chromium.org/3822002/diff/11001/12002#newcode220
> scripts/image_signing/make_dev_firmware.sh:220: 
> Not sure if we really need that (resign kernel on SSD), because that would
> changed the OS image.
> 
> The command is supposed to be executed in developer mode (boot by devinstall
> shim), so you can still boot SSD in dev mode, or boot with USB and install a
new
> OS without dev mode.
> 
> But I agree an option that also updates SSD may be helpful to some use-cases.
> Maybe put that in another CL? (Or do yo think that should be in the first
> release of this tool?)

[gauravsh, 2010-10-14 19:21:07]

lgtm

My main comment is about clearly demarcating this script from the other signing
and image manipulation scripts, since its purpose is different - otherwise over
time, I suspect we will have a crosutils.git like situation where all scripts
end up in the image_signing folder. Let's discuss offline about this.

http://codereview.chromium.org/3822002/diff/17001/18002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/17001/18002#newcode1
scripts/image_signing/make_dev_firmware.sh:1: #!/bin/sh
I think this is maybe a good time to re-factor the structure of script folders
to clearly demarcate scripts that are run on a live system/from the image vs.
those that are meant to work on final images.

In particular, I would rather that the the signing scripts remain standalone as
they are (which means not having the dependency on shflags which they don't use
anyway).

We can discuss offline about my rationale for keeping it that way, but thought I
might as well bring it up.

http://codereview.chromium.org/3822002/diff/17001/18002#newcode165
scripts/image_signing/make_dev_firmware.sh:165: # valid, so that we can know
both they key, vbutil_firmware are working fine.
s/they/the/

http://codereview.chromium.org/3822002/diff/17001/18002#newcode203
scripts/image_signing/make_dev_firmware.sh:203: debug_msg "Backup files when
reading from system live image."
this should be output only if the next if-conditional succeeds

http://codereview.chromium.org/3822002/diff/17001/18002#newcode227
scripts/image_signing/make_dev_firmware.sh:227: echo "Firmware image '$FLAGS_to'
now uses Developer Keys. HWID: $new_hwid"
"New HWID"?

[Randall Spangler, 2010-10-14 19:46:22]

LGTM

http://codereview.chromium.org/3822002/diff/17001/18002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/17001/18002#newcode17
scripts/image_signing/make_dev_firmware.sh:17: DEFINE_string from "" "Path of
input file (empty for system live firmware)" "f"
"system live firmware" really means "current firmware in EEPROM", right?

[Hung-Te, 2010-10-14 23:45:29]

Yes, because I thought "live firmware" is more natural than using the technical
term "EEPROM". Let me know if you prefer to change the message to EEPROM.

[Hung-Te, 2010-10-14 23:52:35]

http://codereview.chromium.org/3822002/diff/17001/18002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/17001/18002#newcode165
scripts/image_signing/make_dev_firmware.sh:165: # valid, so that we can know
both they key, vbutil_firmware are working fine.
oops, I'll fix that later

http://codereview.chromium.org/3822002/diff/17001/18002#newcode203
scripts/image_signing/make_dev_firmware.sh:203: debug_msg "Backup files when
reading from system live image."
This message only appears in debug mode, and for reporting the progress so we
can debug if the checks are working fine, so I think it's OK to keep it here.

http://codereview.chromium.org/3822002/diff/17001/18002#newcode227
scripts/image_signing/make_dev_firmware.sh:227: echo "Firmware image '$FLAGS_to'
now uses Developer Keys. HWID: $new_hwid"
On 2010/10/14 19:21:07, gauravsh wrote:
> "New HWID"?
Yes, but that would exceed column 80, and it seems fine to simply say HWID here
:)

[Hung-Te, 2010-10-15 00:28:52]

http://codereview.chromium.org/3822002/diff/17001/18002
File scripts/image_signing/make_dev_firmware.sh (right):

http://codereview.chromium.org/3822002/diff/17001/18002#newcode1
scripts/image_signing/make_dev_firmware.sh:1: #!/bin/sh
Just want to comment, this script can run on a live system but it also works
fine on the host machine to process a firmware image.

But the main purpose of this script is still to run for live system - so I agree
refactoring should be fine.