Add support for using external signing application and .pem private key files to vbutil_keyblock.

Add support for using external signing application and .pem private key files to vbutil_keyblock.

This allows signing using a .pem file using an external program. 

It is assumed that the external program reads input from stdin, and outputs signed data on stdout. It takes one argument - the file name for the .pem private key reference. See external_rsa_signer.sh for an example external program.

Example usage:
vbutil_keyblock --pack 4096.keyblock \
                --datapubkey 4096.vbpubk \ 
                --signprivate_pem 4096.pem \
                --pem_algorithm 8 \
                --externalsigner "external_rsa_signer.sh"

I have tried to make the change such that it doesn't impact existing tools/interfaces (since these are used at various places). That said, I am aware of the places where we could just extend an old interface an avoid code duplication but thought I'd put that re-factoring in as a TODO for now. Let me know if you disagree and I can merge them (and changing the existing interface).

BUG=7576
TEST=Extended run_vbutil_tests.sh to test vbutil_keyblock packing using an external signer. 

To test, make && make runtests (or just run tests/gen_test_keys.sh; tests/run_vbutils_tests.sh)

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=068fc6f

[Bill Richardson, 2010-10-28 16:06:09]

One complaint, a few nits.

http://codereview.chromium.org/4194003/diff/4001/5004
File host/lib/host_signature.c (right):

http://codereview.chromium.org/4194003/diff/4001/5004#newcode151
host/lib/host_signature.c:151: int fd1[2], fd2[2];
I'd suggest renaming these as 

int p_to_c[2], c_to_p[2];

Then the parent writes to p_to_c[STDOUT_FILENO] and reads from
c_to_p[STDIN_FILENO], and the child likewise. That's easier to understand than a
bunch of integers.

http://codereview.chromium.org/4194003/diff/4001/5004#newcode181
host/lib/host_signature.c:181: /* TODO(gauravsh): Handle read() buffering issues
here. Ideally
Go ahead and fix this now, please. It's only a few lines and will take less time
than debugging the first failure.

[Randall Spangler, 2010-10-28 18:33:43]

http://codereview.chromium.org/4194003/diff/4001/5004
File host/lib/host_signature.c (right):

http://codereview.chromium.org/4194003/diff/4001/5004#newcode156
host/lib/host_signature.c:156: "OUtput of the signer will be read from standar
out.\n",
OU -> Ou
standar --> +d

http://codereview.chromium.org/4194003/diff/4001/5007
File utility/vbutil_keyblock.c (right):

http://codereview.chromium.org/4194003/diff/4001/5007#newcode278
utility/vbutil_keyblock.c:278: fprintf(stderr, "--pem_algorithm must be used
with --signprivate_pem"
This forces --pem_algorithm to be a later argument than --signprivate_pem.  

Would be a little cleaner to do this check before if(parse_error) down on line
303.

Similarly, you could condense the duplicate-ish checks for signing modes above
to a single if (signprivate && signprivate_pem) check below.

[gauravsh, 2010-10-28 21:29:28]

http://codereview.chromium.org/4194003/diff/4001/5004
File host/lib/host_signature.c (right):

http://codereview.chromium.org/4194003/diff/4001/5004#newcode151
host/lib/host_signature.c:151: int fd1[2], fd2[2];
On 2010/10/28 16:06:09, Bill Richardson wrote:
> I'd suggest renaming these as 
> 
> int p_to_c[2], c_to_p[2];
> 
> Then the parent writes to p_to_c[STDOUT_FILENO] and reads from
> c_to_p[STDIN_FILENO], and the child likewise. That's easier to understand than
a
> bunch of integers.
> 
> 

Good idea. Done.

http://codereview.chromium.org/4194003/diff/4001/5004#newcode156
host/lib/host_signature.c:156: "OUtput of the signer will be read from standar
out.\n",
On 2010/10/28 18:33:43, Randall Spangler wrote:
> OU -> Ou
> standar --> +d

Done.

http://codereview.chromium.org/4194003/diff/4001/5004#newcode181
host/lib/host_signature.c:181: /* TODO(gauravsh): Handle read() buffering issues
here. Ideally
On 2010/10/28 16:06:09, Bill Richardson wrote:
> Go ahead and fix this now, please. It's only a few lines and will take less
time
> than debugging the first failure.

Done.

http://codereview.chromium.org/4194003/diff/4001/5007
File utility/vbutil_keyblock.c (right):

http://codereview.chromium.org/4194003/diff/4001/5007#newcode278
utility/vbutil_keyblock.c:278: fprintf(stderr, "--pem_algorithm must be used
with --signprivate_pem"
On 2010/10/28 18:33:43, Randall Spangler wrote:
> This forces --pem_algorithm to be a later argument than --signprivate_pem.  
> 
> Would be a little cleaner to do this check before if(parse_error) down on line
> 303.

I couldn't think of a way to check if pem_algorithm option was specified except
using a auxiliary flag variable. Done.

> 
> Similarly, you could condense the duplicate-ish checks for signing modes above
> to a single if (signprivate && signprivate_pem) check below.

Good idea. Done.

[Bill Richardson, 2010-10-28 22:07:36]

bzzzt! Survey says "X".

http://codereview.chromium.org/4194003/diff/11001/12004
File host/lib/host_signature.c (right):

http://codereview.chromium.org/4194003/diff/11001/12004#newcode182
host/lib/host_signature.c:182: ltr = outbufsize - n;
Oopsie. This is wrong, because if we go through the loop more than once we're
still asking for the original outbufsize-n bytes. Try this:

do {
  int n = read(c_to_p[STDIN_FILENO], outbuf, outbufsize);
  outbuf += n;
  outbufsize -= n;
while (n>0 && outbufsize);

[Randall Spangler, 2010-10-28 22:13:57]

LGTM for my requested fixes (wait for Bill's LGTM for his requests, tho)

[gauravsh, 2010-10-28 22:46:19]

Used your fix. Re-ran vbutil_tests.

http://codereview.chromium.org/4194003/diff/11001/12004
File host/lib/host_signature.c (right):

http://codereview.chromium.org/4194003/diff/11001/12004#newcode182
host/lib/host_signature.c:182: ltr = outbufsize - n;
On 2010/10/28 22:07:36, Bill Richardson wrote:
> Oopsie. This is wrong, because if we go through the loop more than once we're
> still asking for the original outbufsize-n bytes. Try this:
> 
> do {
>   int n = read(c_to_p[STDIN_FILENO], outbuf, outbufsize);
>   outbuf += n;
>   outbufsize -= n;
> while (n>0 && outbufsize);

Ahh yes, you are right. Your version is much cleaner too. Changed.

[Bill Richardson, 2010-10-29 06:22:25]

LGTM