Enforce variations signature verification.

Enforce variations signature verification.

If a seed is received that fails verification, don't store it.
Similarly, on startup discard the seed if it fails verification.

Refactors some of the code to make tests pass (since they're
not testing signed data). Additionally, deprecates the previous
seed hashing code.

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

[jwd, 2014-03-05 17:56:26]

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:111: VLOG(1) <<
"Variations seed signature in local pref missing or invalid "
Do these need to be VLOGs?

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:187:
VariationsSeedStore::VerifySeedSignature(
Can this be tested? What about having a testing key pair or having a pre-signed
base-64 blob in the test?

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.h:63: // signature
verification or VARIATIONS_SEED_SIGNATURE_ENUM_SIZE if signature
opt nit: Overloading VARIATIONS_SEED_SIGNATURE_ENUM_SIZE like this bugs me a
bit.

[Alexei Svitkine (slow), 2014-03-05 18:21:42]

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:111: VLOG(1) <<
"Variations seed signature in local pref missing or invalid "
On 2014/03/05 17:56:27, Jesse Doherty wrote:
> Do these need to be VLOGs?

I've followed the convention that's used in this file already. What alternative
do you propose?

1. Change them to LOG?
2. Change them to DVLOG?
3. Not have these at all - and let users debug via
chrome://histograms/Variations?
4. Something else?

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:187:
VariationsSeedStore::VerifySeedSignature(
On 2014/03/05 17:56:27, Jesse Doherty wrote:
> Can this be tested? What about having a testing key pair or having a
pre-signed
> base-64 blob in the test?

A testing key pair would require passing the public key as a param and also
writing C++ signing code, so I'd rather not go that route. We can add some
base-64 blobs to the test though (one for the payload and one for the
signature). I'm okay doing that, though I'll probably need to generate something
smaller than the current variations seed.

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.h:63: // signature
verification or VARIATIONS_SEED_SIGNATURE_ENUM_SIZE if signature
On 2014/03/05 17:56:27, Jesse Doherty wrote:
> opt nit: Overloading VARIATIONS_SEED_SIGNATURE_ENUM_SIZE like this bugs me a
> bit. 

I considered making this function return a bool and having the result be an out
param, but felt that would make code using this more complicated.

The other alternative is to add an extra enum value, but since it's an UMA enum
now we either have to log the histogram with that value on mobile platforms
(will use more memory) or have an entry in the enum that's never logged, which
is kind of confusing, though probably OK as long as we document it in
histograms.xml. Do you prefer I change it to that?

[jwd, 2014-03-05 18:35:59]

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:111: VLOG(1) <<
"Variations seed signature in local pref missing or invalid "
On 2014/03/05 18:21:43, Alexei Svitkine wrote:
> On 2014/03/05 17:56:27, Jesse Doherty wrote:
> > Do these need to be VLOGs?
> 
> I've followed the convention that's used in this file already. What
alternative
> do you propose?
> 
> 1. Change them to LOG?
> 2. Change them to DVLOG?
> 3. Not have these at all - and let users debug via
> chrome://histograms/Variations?
> 4. Something else?

Ah, didn't notice the convention. I was thinking DVLOG.

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:187:
VariationsSeedStore::VerifySeedSignature(
On 2014/03/05 18:21:43, Alexei Svitkine wrote:
> On 2014/03/05 17:56:27, Jesse Doherty wrote:
> > Can this be tested? What about having a testing key pair or having a
> pre-signed
> > base-64 blob in the test?
> 
> A testing key pair would require passing the public key as a param and also
> writing C++ signing code, so I'd rather not go that route. We can add some
> base-64 blobs to the test though (one for the payload and one for the
> signature). I'm okay doing that, though I'll probably need to generate
something
> smaller than the current variations seed.

Base-64 blob seems fine then.

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.h:63: // signature
verification or VARIATIONS_SEED_SIGNATURE_ENUM_SIZE if signature
On 2014/03/05 18:21:43, Alexei Svitkine wrote:
> On 2014/03/05 17:56:27, Jesse Doherty wrote:
> > opt nit: Overloading VARIATIONS_SEED_SIGNATURE_ENUM_SIZE like this bugs me a
> > bit. 
> 
> I considered making this function return a bool and having the result be an
out
> param, but felt that would make code using this more complicated.
> 
> The other alternative is to add an extra enum value, but since it's an UMA
enum
> now we either have to log the histogram with that value on mobile platforms
> (will use more memory) or have an entry in the enum that's never logged, which
> is kind of confusing, though probably OK as long as we document it in
> histograms.xml. Do you prefer I change it to that?

No, not really. It's probably fine how it is.

[Alexei Svitkine (slow), 2014-03-06 18:16:06]

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/183003008/diff/60001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:187:
VariationsSeedStore::VerifySeedSignature(
On 2014/03/05 18:36:00, Jesse Doherty wrote:
> On 2014/03/05 18:21:43, Alexei Svitkine wrote:
> > On 2014/03/05 17:56:27, Jesse Doherty wrote:
> > > Can this be tested? What about having a testing key pair or having a
> > pre-signed
> > > base-64 blob in the test?
> > 
> > A testing key pair would require passing the public key as a param and also
> > writing C++ signing code, so I'd rather not go that route. We can add some
> > base-64 blobs to the test though (one for the payload and one for the
> > signature). I'm okay doing that, though I'll probably need to generate
> something
> > smaller than the current variations seed.
> 
> Base-64 blob seems fine then.

Done. Added checking for all the possible return values.

[jwd, 2014-03-07 16:11:40]

lgtm

[Alexei Svitkine (slow), 2014-03-07 16:28:43]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/183003008/130001

[commit-bot: I haz the power, 2014-03-07 16:30:57]

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

[commit-bot: I haz the power, 2014-03-07 16:30:58]

Try jobs failed on following builders: mac_chromium_compile_dbg,
mac_chromium_rel

[Alexei Svitkine (slow), 2014-03-07 16:33:18]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:33:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/183003008/130001

[commit-bot: I haz the power, 2014-03-07 20:15:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/183003008/130001

[commit-bot: I haz the power, 2014-03-07 20:32:42]

Change committed as 255697