Do a better job at faking simple challenge signatures.

Do a better job at faking simple challenge signatures.

By returning a signed simple challenge that can actually be parsed as SignedData, we allow callers of the FakeCryptohomeClient to extract the original data back and process it, making for better fake behavior and simpler tests.

Note that the signature is purposedly not verifiable in the FakeCryptohomeClient.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass

Committed: https://crrev.com/911199cb30f608636c79d47e50dca84735e3a08f
Cr-Commit-Position: refs/heads/master@{#417118}

[The one and only Dr. Crash, 2016-09-02 04:38:53]

Description was changed from

==========
Do a better job at faking simple challenge signatures.

BUG=643245
TEST=unit_tests and components_unittests all pass
==========

to

==========
Do a better job at faking simple challenge signatures.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass
==========

[The one and only Dr. Crash, 2016-09-02 04:39:06]

drcrash@chromium.org changed reviewers:
+ dkrahn@chromium.org, xiyuan@chromium.org

[The one and only Dr. Crash, 2016-09-02 04:47:11]

Description was changed from

==========
Do a better job at faking simple challenge signatures.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass
==========

to

==========
Do a better job at faking simple challenge signatures.

By returning a signed simple challenge that can actually be parsed as
SignedData, we allow callers of the FakeCryptohomeClient to extract the original
data back and process it, making for better fake behavior and simpler tests.

Note that the signature is purposedly not verifiable in the
FakeCryptohomeClient.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass
==========

[Darren Krahn, 2016-09-02 17:32:12]

https://codereview.chromium.org/2297193006/diff/40001/chromeos/dbus/fake_cryp...
File chromeos/dbus/fake_cryptohome_client.h (right):

https://codereview.chromium.org/2297193006/diff/40001/chromeos/dbus/fake_cryp...
chromeos/dbus/fake_cryptohome_client.h:220: static const std::string kSignature;
These constants can go in the cc file in an anonymous namespace (and then drop
the 'static'). Also, make these POD type.

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[The one and only Dr. Crash, 2016-09-02 21:47:47]

I put them in the .h so that the unit test could use them. What would you
suggest otherwise? I am happy to make them private (if not, I am on my
phone here) and friend the test.

On Fri, Sep 2, 2016, 10:32 <dkrahn@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2297193006/diff/40001/chromeos/dbus/fake_cryp...
> File chromeos/dbus/fake_cryptohome_client.h (right):
>
>
>
https://codereview.chromium.org/2297193006/diff/40001/chromeos/dbus/fake_cryp...
> chromeos/dbus/fake_cryptohome_client.h:220: static const std::string
> kSignature;
> These constants can go in the cc file in an anonymous namespace (and
> then drop the 'static'). Also, make these POD type.
>
>
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
>
> https://codereview.chromium.org/2297193006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[The one and only Dr. Crash, 2016-09-03 01:17:37]

Looks much better.

https://codereview.chromium.org/2297193006/diff/40001/chromeos/dbus/fake_cryp...
File chromeos/dbus/fake_cryptohome_client.h (right):

https://codereview.chromium.org/2297193006/diff/40001/chromeos/dbus/fake_cryp...
chromeos/dbus/fake_cryptohome_client.h:220: static const std::string kSignature;
On 2016/09/02 17:32:12, Darren Krahn wrote:
> These constants can go in the cc file in an anonymous namespace (and then drop
> the 'static'). Also, make these POD type.
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Thanks for the pointer. And to think these were POD until Patchset 3 where I
thought I should promote them to std::string. I will also move them back to the
.cc file and have the unit tests only check for what matters, e.g. that the
original data is before the nonce and that the nonce is 20 bytes long (just to
avoid hard to debug issues later for those who hardcode 20 byes like our DM
server).

[The one and only Dr. Crash, 2016-09-03 19:09:43]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-03 19:09:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 19:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-03 19:19:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[The one and only Dr. Crash, 2016-09-03 19:24:16]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-03 19:24:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 20:15:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-03 20:15:57]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-09-06 22:32:32]

lgtm

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
chromeos/dbus/fake_cryptohome_client.cc:30: const char kTwentyBytesNonce[] =
"+addtwentybytesnonce";
nit: const -> constexpr

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
chromeos/dbus/fake_cryptohome_client.cc:33: }
nit: append "// namespace" after the closing }

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
File chromeos/dbus/fake_cryptohome_client_unittest.cc (right):

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
chromeos/dbus/fake_cryptohome_client_unittest.cc:36: void Run() {
nit: We probably can get rid of this method and just do
base::RunLoop().RunUntilIdle(); where this method is called,


e.g.
https://cs.chromium.org/chromium/src/dbus/bus_unittest.cc?rcl=0&l=341

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/proto/at...
File chromeos/dbus/proto/attestation.proto (right):

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/proto/at...
chromeos/dbus/proto/attestation.proto:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: Is this a new file? If so, we should do

// Copyright 2016 The Chromium Authors. All rights reserved.

[The one and only Dr. Crash, 2016-09-07 02:43:47]

Darren, do you have any opinion about the move of attestation.proto? In a way, I
think it could belong in the attestation/ directory (even though none of the
code in there depends on it) just as a "neutral ground" for that proto
definition. But the dbus location is fine too, and flows well dependencies-wise.

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
chromeos/dbus/fake_cryptohome_client.cc:33: }
On 2016/09/06 22:32:32, xiyuan wrote:
> nit: append "// namespace" after the closing }

Done.

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
File chromeos/dbus/fake_cryptohome_client_unittest.cc (right):

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
chromeos/dbus/fake_cryptohome_client_unittest.cc:36: void Run() {
On 2016/09/06 22:32:32, xiyuan wrote:
> nit: We probably can get rid of this method and just do
> base::RunLoop().RunUntilIdle(); where this method is called,
> 
> 
> e.g.
> https://cs.chromium.org/chromium/src/dbus/bus_unittest.cc?rcl=0&l=341

Sure.

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/proto/at...
File chromeos/dbus/proto/attestation.proto (right):

https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/proto/at...
chromeos/dbus/proto/attestation.proto:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2016/09/06 22:32:32, xiyuan wrote:
> nit: Is this a new file? If so, we should do
> 
> // Copyright 2016 The Chromium Authors. All rights reserved.

I moved it from some other location so it's not new (though initially mine was
new,so it is confusing I am sure).

[The one and only Dr. Crash, 2016-09-07 22:16:55]

The CQ bit was checked by drcrash@chromium.org

[The one and only Dr. Crash, 2016-09-07 22:16:56]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2297193006/#ps120001
(title: "Adressed feedback.")

[commit-bot: I haz the power, 2016-09-07 22:17:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 23:43:25]

Description was changed from

==========
Do a better job at faking simple challenge signatures.

By returning a signed simple challenge that can actually be parsed as
SignedData, we allow callers of the FakeCryptohomeClient to extract the original
data back and process it, making for better fake behavior and simpler tests.

Note that the signature is purposedly not verifiable in the
FakeCryptohomeClient.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass
==========

to

==========
Do a better job at faking simple challenge signatures.

By returning a signed simple challenge that can actually be parsed as
SignedData, we allow callers of the FakeCryptohomeClient to extract the original
data back and process it, making for better fake behavior and simpler tests.

Note that the signature is purposedly not verifiable in the
FakeCryptohomeClient.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass
==========

[commit-bot: I haz the power, 2016-09-07 23:43:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-07 23:45:52]

Description was changed from

==========
Do a better job at faking simple challenge signatures.

By returning a signed simple challenge that can actually be parsed as
SignedData, we allow callers of the FakeCryptohomeClient to extract the original
data back and process it, making for better fake behavior and simpler tests.

Note that the signature is purposedly not verifiable in the
FakeCryptohomeClient.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass
==========

to

==========
Do a better job at faking simple challenge signatures.

By returning a signed simple challenge that can actually be parsed as
SignedData, we allow callers of the FakeCryptohomeClient to extract the original
data back and process it, making for better fake behavior and simpler tests.

Note that the signature is purposedly not verifiable in the
FakeCryptohomeClient.

BUG=643245
TEST=chromeos_unittests; unit_tests and components_unittests also pass

Committed: https://crrev.com/911199cb30f608636c79d47e50dca84735e3a08f
Cr-Commit-Position: refs/heads/master@{#417118}
==========

[commit-bot: I haz the power, 2016-09-07 23:45:53]

Patchset 7 (id:??) landed as
https://crrev.com/911199cb30f608636c79d47e50dca84735e3a08f
Cr-Commit-Position: refs/heads/master@{#417118}

[achuithb, 2016-09-08 09:50:08]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2326473002/ by achuith@chromium.org.

The reason for reverting is: crbug.com/645052

This is causing build_package failures on chromeos..

[Darren Krahn, 2016-09-08 22:45:18]

On 2016/09/07 02:43:47, The one and only Dr. Crash wrote:
> Darren, do you have any opinion about the move of attestation.proto? In a way,
I
> think it could belong in the attestation/ directory (even though none of the
> code in there depends on it) just as a "neutral ground" for that proto
> definition. But the dbus location is fine too, and flows well
dependencies-wise.

Sorry for the delay -- FWIW, I don't feel strongly...

> 
>
https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
> File chromeos/dbus/fake_cryptohome_client.cc (right):
> 
>
https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
> chromeos/dbus/fake_cryptohome_client.cc:33: }
> On 2016/09/06 22:32:32, xiyuan wrote:
> > nit: append "// namespace" after the closing }
> 
> Done.
> 
>
https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
> File chromeos/dbus/fake_cryptohome_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/fake_cry...
> chromeos/dbus/fake_cryptohome_client_unittest.cc:36: void Run() {
> On 2016/09/06 22:32:32, xiyuan wrote:
> > nit: We probably can get rid of this method and just do
> > base::RunLoop().RunUntilIdle(); where this method is called,
> > 
> > 
> > e.g.
> > https://cs.chromium.org/chromium/src/dbus/bus_unittest.cc?rcl=0&l=341
> 
> Sure.
> 
>
https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/proto/at...
> File chromeos/dbus/proto/attestation.proto (right):
> 
>
https://codereview.chromium.org/2297193006/diff/100001/chromeos/dbus/proto/at...
> chromeos/dbus/proto/attestation.proto:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> On 2016/09/06 22:32:32, xiyuan wrote:
> > nit: Is this a new file? If so, we should do
> > 
> > // Copyright 2016 The Chromium Authors. All rights reserved.
> 
> I moved it from some other location so it's not new (though initially mine was
> new,so it is confusing I am sure).

[The one and only Dr. Crash, 2016-09-08 22:51:55]

> Sorry for the delay -- FWIW, I don't feel strongly...

Let's leave it in dbus/ for now, and maybe consolidate a bunch of
attestation stuff later on so it can be shared between Chromium and
Chromium OS. Protos and .h derived from copying info from protos are out of
sync again as work on attestationd is progressing. I asked Andrey to file a
tracking bug for that.

On Thu, Sep 8, 2016 at 3:45 PM, <dkrahn@chromium.org> wrote:

> On 2016/09/07 02:43:47, The one and only Dr. Crash wrote:
> > Darren, do you have any opinion about the move of attestation.proto? In
> a way,
> I
> > think it could belong in the attestation/ directory (even though none of
> the
> > code in there depends on it) just as a "neutral ground" for that proto
> > definition. But the dbus location is fine too, and flows well
> dependencies-wise.
>
> Sorry for the delay -- FWIW, I don't feel strongly...
>
> >
> >
> https://codereview.chromium.org/2297193006/diff/100001/
> chromeos/dbus/fake_cryptohome_client.cc
> > File chromeos/dbus/fake_cryptohome_client.cc (right):
> >
> >
> https://codereview.chromium.org/2297193006/diff/100001/
> chromeos/dbus/fake_cryptohome_client.cc#newcode33
> > chromeos/dbus/fake_cryptohome_client.cc:33: }
> > On 2016/09/06 22:32:32, xiyuan wrote:
> > > nit: append "// namespace" after the closing }
> >
> > Done.
> >
> >
> https://codereview.chromium.org/2297193006/diff/100001/
> chromeos/dbus/fake_cryptohome_client_unittest.cc
> > File chromeos/dbus/fake_cryptohome_client_unittest.cc (right):
> >
> >
> https://codereview.chromium.org/2297193006/diff/100001/
> chromeos/dbus/fake_cryptohome_client_unittest.cc#newcode36
> > chromeos/dbus/fake_cryptohome_client_unittest.cc:36: void Run() {
> > On 2016/09/06 22:32:32, xiyuan wrote:
> > > nit: We probably can get rid of this method and just do
> > > base::RunLoop().RunUntilIdle(); where this method is called,
> > >
> > >
> > > e.g.
> > > https://cs.chromium.org/chromium/src/dbus/bus_unittest.cc?rcl=0&l=341
> >
> > Sure.
> >
> >
> https://codereview.chromium.org/2297193006/diff/100001/
> chromeos/dbus/proto/attestation.proto
> > File chromeos/dbus/proto/attestation.proto (right):
> >
> >
> https://codereview.chromium.org/2297193006/diff/100001/
> chromeos/dbus/proto/attestation.proto#newcode1
> > chromeos/dbus/proto/attestation.proto:1: // Copyright (c) 2012 The
> Chromium
> > Authors. All rights reserved.
> > On 2016/09/06 22:32:32, xiyuan wrote:
> > > nit: Is this a new file? If so, we should do
> > >
> > > // Copyright 2016 The Chromium Authors. All rights reserved.
> >
> > I moved it from some other location so it's not new (though initially
> mine was
> > new,so it is confusing I am sure).
>
>
>
> https://codereview.chromium.org/2297193006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.