Finished implementing AttestationPolicyObserver.

Finished implementing AttestationPolicyObserver.

Created a payload protobuf to track whether a key's certificate has
already been uploaded.

BUG=chromium:219959
TEST=unit

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197210

[dkrahn, 2013-04-19 21:54:10]

+jochen as gypi OWNER

[Mattias Nissler (ping if slow), 2013-04-22 10:25:42]

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:182:
base::TimeDelta threshold = base::TimeDelta::FromDays(30);
const

We usually put constant decls in an anonymous namespace at the top of the file.

Also, is 30 maybe a bit on the large side? What validity periods do we expect
people to use?

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:246:
base::Bind(DBusBoolRedirectCallback, base::Closure(), base::Closure()));
Here and elsewhere: Should we have some logging in strategic places to be able
to diagnose failures of various sorts? Communication problems with cryptohome
has been a source of problems in the past.

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:24:
using std::string;
We usually don't do this given the little benefit it provides.

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:71:
scoped_ptr<crypto::RSAPrivateKey> g_private_key;
No non-POD globals please.

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:136:
g_private_key.reset(crypto::RSAPrivateKey::Create(2048));
How costly is this? policy_builder.cc has pre-created keys embedded which you
could potentially use here as well?

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:145:
valid_expiry);
Do we have any suitable certs in chrome/test/data already?

https://codereview.chromium.org/14220003/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:265:
}
And what you're doing here is why some people hate gmock :)

These chains of expectations are hard to read and cause maintenance overhead
(note that you had to update expectation setup in your existing test cases as
well). The usual advice for this situation is to create an actual fake
implementation of CryptohomeClient that you can configure in the test setup.
Admittedly, in case of CryptohomeClient this is a bit painful due to the
interface being so bloated (but that's another rant).

[dkrahn, 2013-04-22 22:33:31]

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:182:
base::TimeDelta threshold = base::TimeDelta::FromDays(30);
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> const
> 
> We usually put constant decls in an anonymous namespace at the top of the
file.
> 
> Also, is 30 maybe a bit on the large side? What validity periods do we expect
> people to use?
> 

Added a constant member to the class.  

The PCA issues certs with at least a 2 year expiry.  I chose 30 days here
because this logic may only run when the user logs in and I want to replace the
majority of certs before they expire.  If I make this only a few days many users
will not logout / login during that period.  However, 30 is still pretty
arbitrary and not backed by any real data.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:246:
base::Bind(DBusBoolRedirectCallback, base::Closure(), base::Closure()));
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> Here and elsewhere: Should we have some logging in strategic places to be able
> to diagnose failures of various sorts? Communication problems with cryptohome
> has been a source of problems in the past.

Added logging to dbus callbacks which reports failed method name.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:24:
using std::string;
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> We usually don't do this given the little benefit it provides.

Done.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:71:
scoped_ptr<crypto::RSAPrivateKey> g_private_key;
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> No non-POD globals please.

Done.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:136:
g_private_key.reset(crypto::RSAPrivateKey::Create(2048));
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> How costly is this? policy_builder.cc has pre-created keys embedded which you
> could potentially use here as well?

Good idea -- used a key from policy_builder.cc.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:145:
valid_expiry);
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> Do we have any suitable certs in chrome/test/data already?

The reason I'm generating dynamically is to fiddle with the X.509 expiry fields
which must be relative to the current time.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:265:
}
On 2013/04/22 10:25:42, Mattias Nissler wrote:
> And what you're doing here is why some people hate gmock :)
> 
> These chains of expectations are hard to read and cause maintenance overhead
> (note that you had to update expectation setup in your existing test cases as
> well). The usual advice for this situation is to create an actual fake
> implementation of CryptohomeClient that you can configure in the test setup.
> Admittedly, in case of CryptohomeClient this is a bit painful due to the
> interface being so bloated (but that's another rant).

Each time I update the CryptohomeClient interface, I dislike the approach in
device_local_account_browsertest.cc which I have to maintain.  However, you're
right about readability and too strictly using gmock.  I've add a SetupMocks()
method which I think makes this more readable and I use strict expectations only
when I'm actually interested in them.

[Mattias Nissler (ping if slow), 2013-04-23 10:53:00]

A couple more nits. I'm willing to grudgingly accept the gmock version for now,
but please to consider the comments first.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:182:
base::TimeDelta threshold = base::TimeDelta::FromDays(30);
On 2013/04/22 22:33:31, Darren Krahn wrote:
> On 2013/04/22 10:25:42, Mattias Nissler wrote:
> > const
> > 
> > We usually put constant decls in an anonymous namespace at the top of the
> file.
> > 
> > Also, is 30 maybe a bit on the large side? What validity periods do we
expect
> > people to use?
> > 
> 
> Added a constant member to the class.  
> 
> The PCA issues certs with at least a 2 year expiry.  I chose 30 days here
> because this logic may only run when the user logs in and I want to replace
the
> majority of certs before they expire.  If I make this only a few days many
users
> will not logout / login during that period.  However, 30 is still pretty
> arbitrary and not backed by any real data.

Fair. Would be good to include this additional context in a comment.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:145:
valid_expiry);
On 2013/04/22 22:33:31, Darren Krahn wrote:
> On 2013/04/22 10:25:42, Mattias Nissler wrote:
> > Do we have any suitable certs in chrome/test/data already?
> 
> The reason I'm generating dynamically is to fiddle with the X.509 expiry
fields
> which must be relative to the current time.

I see.

https://chromiumcodereview.appspot.com/14220003/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:265:
}
On 2013/04/22 22:33:31, Darren Krahn wrote:
> On 2013/04/22 10:25:42, Mattias Nissler wrote:
> > And what you're doing here is why some people hate gmock :)
> > 
> > These chains of expectations are hard to read and cause maintenance overhead
> > (note that you had to update expectation setup in your existing test cases
as
> > well). The usual advice for this situation is to create an actual fake
> > implementation of CryptohomeClient that you can configure in the test setup.
> > Admittedly, in case of CryptohomeClient this is a bit painful due to the
> > interface being so bloated (but that's another rant).
> 
> Each time I update the CryptohomeClient interface, I dislike the approach in
> device_local_account_browsertest.cc which I have to maintain.  

Well, the root cause with the pain here is not gmock being superior, but
CryptohomeClient being a kitchen sink. There's no good reason why it needs to be
that way, in fact we should probably have a separate interface for the
Attestation calls and only the attestation calls, and then it'd be a breeze to
implement a fake here.

> However, you're
> right about readability and too strictly using gmock.  I've add a SetupMocks()
> method which I think makes this more readable and I use strict expectations
only
> when I'm actually interested in them.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_key_payload.proto (right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_key_payload.proto:17: 
remove trailing newline

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:42:
LOG(WARNING) << "Cryptohome DBus method failed: " << method_name;
You probably want to log |status| as well.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:173: "",
DBUS_METHOD_CALL_SUCCESS));
Should have a proper identifying name? FWIW, you could also use FROM_HERE.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:54: static
const char kEnterpriseMachineKey[];
nit: newline before coment

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:57: static
const int kExpiryThresholdInDays;
If you're not using these elsewhere, you can just make them constants local to
the .cc file.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:141:
void SetupMocks(int mock_options, const std::string& certificate) {
I still think a fake would be much easier to follow. What do you think about
breaking the attestation stuff out of CryptohomeClient?

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:167:
.WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
Why should this happen only once?

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:170:
.WillOnce(WithArgs<3>(Invoke(DBusCallbackTrue)));
ditto

[dkrahn, 2013-04-23 21:17:09]

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_key_payload.proto (right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_key_payload.proto:17: 
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> remove trailing newline

Done.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:42:
LOG(WARNING) << "Cryptohome DBus method failed: " << method_name;
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> You probably want to log |status| as well.

Done.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:173: "",
DBUS_METHOD_CALL_SUCCESS));
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> Should have a proper identifying name? FWIW, you could also use FROM_HERE.

Good idea, changed to use FROM_HERE.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:54: static
const char kEnterpriseMachineKey[];
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> nit: newline before coment

Done.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:57: static
const int kExpiryThresholdInDays;
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> If you're not using these elsewhere, you can just make them constants local to
> the .cc file.

Done.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:141:
void SetupMocks(int mock_options, const std::string& certificate) {
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> I still think a fake would be much easier to follow. What do you think about
> breaking the attestation stuff out of CryptohomeClient?

Logged crbug.com/234759 to break up CryptohomeClient.

I agree, the way CryptohomeClient is using gmock it would be a good to have a
fake.  But the requirements here are so simple and the barrier so high to
creating a fake for this class that the trade-off is not worth it IMHO.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:167:
.WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> Why should this happen only once?

Uploading to DMServer is expensive and should only be done when necessary. 
During a single run through the logic in this class there is no reason to upload
multiple times.  Added comment to clarify.

https://chromiumcodereview.appspot.com/14220003/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:170:
.WillOnce(WithArgs<3>(Invoke(DBusCallbackTrue)));
On 2013/04/23 10:53:01, Mattias Nissler wrote:
> ditto

This is the 'uploaded=true' indicator and writing it should be paired with the
upload.

[Mattias Nissler (ping if slow), 2013-04-25 12:08:08]

LGTM

[jochen (gone - plz use gerrit), 2013-04-25 13:19:50]

lgtm

[commit-bot: I haz the power, 2013-04-25 22:53:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/14220003/14001

[commit-bot: I haz the power, 2013-04-25 22:53:12]

Failed to apply patch for chrome/chrome_tests_unit.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_tests_unit.gypi
  Hunk #1 FAILED at 523.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_tests_unit.gypi.rej

Patch:       chrome/chrome_tests_unit.gypi
Index: chrome/chrome_tests_unit.gypi
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index
d713109ad5786f6b8214d8448c55b00236a5118a..a7d5d8dd7cdf40fd6de511512ad3fbbe2e17f07f
100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -523,7 +523,7 @@
         'browser/chrome_page_zoom_unittest.cc',
         'browser/chrome_process_singleton_win_unittest.cc',
         'browser/chromeos/accessibility/magnification_manager_unittest.cc',
-	'browser/chromeos/attestation/attestation_policy_observer_unittest.cc',
+        'browser/chromeos/attestation/attestation_policy_observer_unittest.cc',
         'browser/chromeos/contacts/contact_database_unittest.cc',
         'browser/chromeos/contacts/contact_manager_stub.cc',
         'browser/chromeos/contacts/contact_manager_stub.h',

[commit-bot: I haz the power, 2013-04-26 00:06:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/14220003/21001

[commit-bot: I haz the power, 2013-04-26 00:26:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[dkrahn, 2013-04-26 21:59:30]

+wtc as OWNER of net/

Added NET_EXPORT_PRIVATE to CreateSelfSignedCert for use in
attestation_policy_observer_unittest.cc.

[wtc, 2013-04-27 17:24:29]

Patch set 5 LGTM. I only reviewed net/cert/x509_util_nss.h.

https://codereview.chromium.org/14220003/diff/39001/net/cert/x509_util_nss.h
File net/cert/x509_util_nss.h (right):

https://codereview.chromium.org/14220003/diff/39001/net/cert/x509_util_nss.h#...
net/cert/x509_util_nss.h:12: #include "net/cert/x509_certificate.h"

Nit: it would be good to include "net/base/net_export.h"
explicitly.

[dkrahn, 2013-04-29 21:34:56]

https://chromiumcodereview.appspot.com/14220003/diff/39001/net/cert/x509_util...
File net/cert/x509_util_nss.h (right):

https://chromiumcodereview.appspot.com/14220003/diff/39001/net/cert/x509_util...
net/cert/x509_util_nss.h:12: #include "net/cert/x509_certificate.h"
On 2013/04/27 17:24:30, wtc wrote:
> 
> Nit: it would be good to include "net/base/net_export.h"
> explicitly.

Done.

[commit-bot: I haz the power, 2013-04-29 21:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/14220003/45001

[commit-bot: I haz the power, 2013-04-30 01:15:14]

Change committed as 197210