service,cryptohome: wire up lockbox to dbus

service,cryptohome: wire up lockbox to dbus

Lockbox provides install-time, tamper evident data.  This change wires it up to DBus.

BUG=chromium-os:13129
TEST=clear the tpm and reboot
          check /var/log/cryptohomed.log for Init message
          run cryptohome --action=status and check lockbox output
          run cryptohome --action=lockbox_test
          run cryptohome --action=status and check new fields exist
          reboot
          run cryptohome --action=status and ensure it is locked and has the data

I haven't yet figured out how to best integrate this into an autotest.  Basically, we need to save the TPM Owner password to disk and be able to tell cryptohomed to load it on demand to simulate a TpmClear event.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=2b5cdc3

[Will Drewry, 2011-04-07 22:24:04]

PTAL! (3/3)

[gauravsh, 2011-04-11 04:03:20]

Sorry for doing this out of order. That's a quite a lot of code. :) I am doing
quick passes first - will try to get a fuller review in the next day or 2.

http://codereview.chromium.org/6801020/diff/7002/service.cc
File service.cc (right):

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode113
service.cc:113: // be called across multiple threads in parallel.
Is this comment for InitializeLockBox()? If so, can you add the comment in the
header definition too.

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode168
service.cc:168: void Service::InitializeLockbox() {
Should this return true or false? Otherwise the caller will never know if the
initialization succeeded?

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode722
service.cc:722: lockbox_data += StringPrintf(
is using += as efficient as using append()? (not that efficiency may matter that
much here).

http://codereview.chromium.org/6801020/diff/7002/service.h
File service.h (right):

http://codereview.chromium.org/6801020/diff/7002/service.h#newcode20
service.h:20: #include "lockbox.h"
nit: alpha

http://codereview.chromium.org/6801020/diff/7002/service.h#newcode196
service.h:196: cryptohome::Lockbox *lockbox_;
nit: * next to the type.

[Will Drewry, 2011-04-13 16:04:59]

PTAL - thanks!

http://codereview.chromium.org/6801020/diff/7002/service.cc
File service.cc (right):

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode113
service.cc:113: // be called across multiple threads in parallel.
On 2011/04/11 04:03:20, gauravsh wrote:
> Is this comment for InitializeLockBox()? If so, can you add the comment in the
> header definition too.

The part that is relevant (thread safety) is in the header for both
InstallAttributes and Lockbox.

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode168
service.cc:168: void Service::InitializeLockbox() {
On 2011/04/11 04:03:20, gauravsh wrote:
> Should this return true or false? Otherwise the caller will never know if the
> initialization succeeded?

The caller doesn't matter as much unless we want cryptohome to do the freaking
out.  We haven't yet decided if cryptohomed, session_manager, or CHrome should
freak out if the InstallAttributes are broken.  THat said, they fail such that
they look empty so it's "safe" to just hang out.

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode722
service.cc:722: lockbox_data += StringPrintf(
On 2011/04/11 04:03:20, gauravsh wrote:
> is using += as efficient as using append()? (not that efficiency may matter
that
> much here).

No idea.  Done.

[gauravsh, 2011-04-14 07:39:52]

http://codereview.chromium.org/6801020/diff/7002/service.cc
File service.cc (right):

http://codereview.chromium.org/6801020/diff/7002/service.cc#newcode722
service.cc:722: lockbox_data += StringPrintf(
On 2011/04/13 16:04:59, Will Drewry wrote:
> On 2011/04/11 04:03:20, gauravsh wrote:
> > is using += as efficient as using append()? (not that efficiency may matter
> that
> > much here).
> 
> No idea.  Done.

Yeah, it seems either append or += is ok. What's not ok is using +. That creates
temp copies and is a waste of energy.

http://codereview.chromium.org/6801020/diff/19009/cryptohome.cc
File cryptohome.cc (right):

http://codereview.chromium.org/6801020/diff/19009/cryptohome.cc#newcode70
cryptohome.cc:70: ACTION_INSTALL_ATTRIBUTES_TEST };
Can you make this more general instead of making this a test driver. Something
like.

ACTION_INSTALL_ATTRIBUTES_INIT
ACTION_INSTALL_ATTRIBUTES_ADD
ACTION_INSTALL_ATTRIBUTES_LIST
ACTION_INSTALL_ATTRIBUTES_FINALIZE

I think it'd be useful for your testing as well be consistent of how all the
other options for cryptohome --action work.

http://codereview.chromium.org/6801020/diff/19009/cryptohome.cc#newcode585
cryptohome.cc:585: if (serialized.has_last_activity_timestamp()) {
believe this is not a part of your CL, right?

http://codereview.chromium.org/6801020/diff/19009/cryptohome.xml
File cryptohome.xml (right):

http://codereview.chromium.org/6801020/diff/19009/cryptohome.xml#newcode118
cryptohome.xml:118: <method name="InstallAttributesGet">
yeah, so basically add an --action for each of these or those you think make
sure. I think that'd be consistent with how all the --action are used.

http://codereview.chromium.org/6801020/diff/19009/service.h
File service.h (right):

http://codereview.chromium.org/6801020/diff/19009/service.h#newcode214
service.h:214: cryptohome::InstallAttributes *install_attrs_;
nit: * next to type

[Will Drewry, 2011-04-15 01:31:48]

PTAL - Thanks!

http://codereview.chromium.org/6801020/diff/19009/cryptohome.cc
File cryptohome.cc (right):

http://codereview.chromium.org/6801020/diff/19009/cryptohome.cc#newcode70
cryptohome.cc:70: ACTION_INSTALL_ATTRIBUTES_TEST };
On 2011/04/14 07:39:52, gauravsh wrote:
> Can you make this more general instead of making this a test driver. Something
> like.
> 
> ACTION_INSTALL_ATTRIBUTES_INIT
> ACTION_INSTALL_ATTRIBUTES_ADD
> ACTION_INSTALL_ATTRIBUTES_LIST
> ACTION_INSTALL_ATTRIBUTES_FINALIZE
> 
> I think it'd be useful for your testing as well be consistent of how all the
> other options for cryptohome --action work.

Done.

http://codereview.chromium.org/6801020/diff/19009/cryptohome.cc#newcode585
cryptohome.cc:585: if (serialized.has_last_activity_timestamp()) {
On 2011/04/14 07:39:52, gauravsh wrote:
> believe this is not a part of your CL, right?

Done.

http://codereview.chromium.org/6801020/diff/19009/cryptohome.xml
File cryptohome.xml (right):

http://codereview.chromium.org/6801020/diff/19009/cryptohome.xml#newcode118
cryptohome.xml:118: <method name="InstallAttributesGet">
On 2011/04/14 07:39:52, gauravsh wrote:
> yeah, so basically add an --action for each of these or those you think make
> sure. I think that'd be consistent with how all the --action are used.

Done. Enough at least :)

http://codereview.chromium.org/6801020/diff/19009/service.h
File service.h (right):

http://codereview.chromium.org/6801020/diff/19009/service.h#newcode214
service.h:214: cryptohome::InstallAttributes *install_attrs_;
On 2011/04/14 07:39:52, gauravsh wrote:
> nit: * next to type

Done, and fixed up all the other ones.

[gauravsh, 2011-04-15 02:37:50]

lgtm