Add public key and signature verification to browser-side API keys

content/common/experiments/api_key.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_COMMON_EXPERIMENTS_API_KEY_H_
 #define CONTENT_COMMON_EXPERIMENTS_API_KEY_H_
 
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
@@ skipping 38 matching lines @@
   GURL origin() { return origin_; }
   std::string api_name() { return api_name_; }
   uint64_t expiry_timestamp() { return expiry_timestamp_; }
 
  protected:
   friend class ApiKeyTest;
 
   bool ValidateOrigin(const std::string& origin) const;
   bool ValidateApiName(const std::string& api_name) const;
   bool ValidateDate(const base::Time& now) const;
+  bool ValidateSignature(const uint8_t* public_key) const;

[davidben, 2016/01/11 20:18:57]

Passing in a bare pointer without some indication of length seems iffy. I would
suggest one of:
- Use a base::StringPiece and check that the length is 32 in the function.
- Make the parameter a const uint8_t public_key[32] which at least tells the
reader they're responsible for passing in something of suitable size. (This
doesn't make a copy. C is weird.)

[iclelland, 2016/01/12 14:52:49]

I can certainly switch to StringPiece if that makes you feel better about it :)
The code is only called by this class, with a reference to the included public
key, and by tests, with a pointer to the public key in *that* class. It's not a
public method.

In the future, though, it will need to deal with data provided by the /chrome/
layer, so I may as well switch it now. 

(I had a length parameter, but it wasn't being used in any meaningful way, so it
was removed. Also, annotating the parameter size in the declaration looks like a
fun hack, but it's really no better than a code comment, I think.)

[davidben, 2016/01/13 20:50:39]

I'm fine with whatever. StringPiece or code comment or the funny array thing.
Also occurs to me that const uint8_t (&public_key)[32] would let the compiler
check it.

*shrug* I just wanted there to be some indication as to what the size is
supposed to be. Being protected, it's part of your class's API for (test-only)
subclasses.

[iclelland, 2016/01/13 21:30:11]

I'm good with StringPiece (other than all of the reinterpret_casts needed to use
it to uint_8 data). It may evolve into a PublicKey class at some point, but for
now it's just good to be able to pass around a single *thing* with an intrinsic
length property.

[davidben, 2016/01/13 22:18:13]

Yeah, having to add those casts drives me crazy. :-) (I will never forgive you,
C, for signed char...)

There's https://crbug.com/559302 but it's still unresolved.

+
+  static bool ValidateSignature(const std::string& signature_text,
+                                const std::string& data,
+                                const uint8_t* public_key);
 
  private:
   ApiKey(const std::string& signature,
          const std::string& data,
          const GURL& origin,
          const std::string& api_name,
          uint64_t expiry_timestamp);
 
   // The base64-encoded-signature portion of the key. For the key to be valid,
   // this must be a valid signature for the data portion of the key, as verified
@@ skipping 11 matching lines @@
   std::string api_name_;
 
   // The time until which this key should be considered valid, in UTC, as
   // seconds since the Unix epoch.
   uint64_t expiry_timestamp_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_EXPERIMENTS_API_KEY_H_