Enable passing cast channel certificate authority keys.

extensions/browser/api/cast_channel/cast_auth_util.h

 // Copyright 2014 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 EXTENSIONS_BROWSER_API_CAST_CHANNEL_CAST_AUTH_UTIL_H_
 #define EXTENSIONS_BROWSER_API_CAST_CHANNEL_CAST_AUTH_UTIL_H_
 
 #include <string>

[Ryan Sleevi, 2014/10/17 19:53:20]

STYLE: Newline between lines 8 and 9

[vadimgo, 2014/10/20 23:35:27]

Done.

+#include "crypto/scoped_nss_types.h"

[Ryan Sleevi, 2014/10/17 19:53:20]

This is a non _nss.h file. Do not use NSS types directly.

From your use, it would seem you could just use a base::StringPiece

[vadimgo, 2014/10/20 23:35:27]

Done.

+#include "extensions/common/api/cast_channel/authority_keys.pb.h"

[Ryan Sleevi, 2014/10/17 19:53:21]

STRONGLY discourage including .pb.h in .h files, especially if they're public.
It opens up a host of dependency issues (in that all dependents that #include
this header must have their GYP targets properly configured to wait for the
protoc step to finish)

[vadimgo, 2014/10/20 23:35:27]

Done.

+#include "net/base/hash_value.h"
 
 namespace extensions {
 namespace core_api {
 namespace cast_channel {
 
 class CastMessage;
 
 struct AuthResult {
  public:
   enum ErrorType {
@@ skipping 26 matching lines @@
   std::string error_message;
   ErrorType error_type;
   int nss_error_code;
 
  private:
   AuthResult(const std::string& error_message,
              ErrorType error_type,
              int nss_error_code);
 };
 
+// Helper class for intermediate certificate authority validation.

[Ryan Sleevi, 2014/10/17 19:53:21]

DOCUMENTATION: This comment doesn't really provide any guidance to the reader as
to what this class is for or how it can be used (note: not how it _is_ used, how
it _can be_ used)

I've attempted a small wording, but this is based on _only_ looking at the
header here, so please make changes and adjust appropriately.

// AuthorityKeysStore is a helper class that is used to store and
// manipulate the set of intermediate CAs (ICAs) that are used with authorized
// CAST devices.
// A static list of ICAs is hardcoded, but may optionally be updated
// during runtime by an extension supplying a protobuf of ICAs signed
// with a well-known key.

[vadimgo, 2014/10/20 23:35:27]

Done.

+class AuthorityKeysStore {
+ public:
+  AuthorityKeysStore();
+  ~AuthorityKeysStore();
+
+  // Returns the public key of the ICA whose fingerprint matches |fingerprint|.
+  // Returns NULL, if no such ICA is found.
+  const SECItem* GetICAPublicKeyFromFingerprint(
+      const net::SHA256HashValue& fingerprint);
+
+  // Returns the public key of the first ICA in the list.
+  // Returns NULL if the authority keys store is empty.
+  const SECItem* GetDefaultICAPublicKey();

[Ryan Sleevi, 2014/10/17 19:53:21]

DOCUMENTATION: This documentation and method name surprise me. It's unclear
when/how this would ever be used. If this is meant for the legacy case, then the
position in or presence of the list should be irrelevant. I'm sure this will be
clearer when I read the .cc, but take this as an item to improve.

[vadimgo, 2014/10/20 23:35:27]

Done.

+
+  // Loads authority keys from a serialized protobuf.
+  bool Load(std::string& keys);

[Ryan Sleevi, 2014/10/17 19:53:21]

STYLE: Passing non-const references is forbidden
DESIGN: Is passing an std::string the right type? Should it be the caller's
responsibility to deserialize this? Alternatively, should this be a
base::StringPiece instead, to allow zero-copy deserialization?

[vadimgo, 2014/10/20 23:35:27]

Done.

+
+ private:
+  // Info for trusted ICA certs.

[Ryan Sleevi, 2014/10/17 19:53:21]

DOCUMENTATION: I suspect you shouldn't have struct-level documentation, but
should instead document the members

STYLE: You can forward declare this, because the vector instantiation on line 85
is out of line (lines 61/62)

[vadimgo, 2014/10/20 23:35:27]

Done.

+  struct ICACertInfo {
+    const net::SHA256HashValue* fingerprint;
+    SECItem public_key;
+  };
+
+  // The list of trusted certificate authorities, this points to either
+  // hard-coded constant data or to the data in the de-serialized protobuf.
+  std::vector<ICACertInfo> certificate_authorities_;

[Ryan Sleevi, 2014/10/17 19:53:21]

DESIGN: You are effectively using a vector of tuples, and then finding on the
first. That's generally known as a map. The only reason it seems you didn't use
a map is because you store fingerprint by pointer, rather than by value. But
SHA256HashValue is a POD designed to be implicit-copyable AND for use in
associative containers, so why can't/didn't you just do

std::map<net::SHA256HashValue, base::StringPiece, net::SHA256HashValueLessThan>
?

[vadimgo, 2014/10/20 23:35:27]

Done.

+
+  // Holds hard-coded constant ICA data.
+  static const ICACertInfo kAllowedICAs[];

[Ryan Sleevi, 2014/10/17 19:53:21]

STYLE: Don't ever declare private static class data members unless necessary for
unit testing (and boo then).

They can always be handled by a name in an unnamed namespace in a .cc file.

[vadimgo, 2014/10/20 23:35:27]

Done.

+
+  // Trusted certificate authorities data passed from the extension.
+  proto::AuthorityKeys authority_keys_;
+};

[Ryan Sleevi, 2014/10/17 19:53:21]

STYLE: DISALLOW_COPY_AND_ASSIGN

[vadimgo, 2014/10/20 23:35:27]

Done.

+
 // Authenticates the given |challenge_reply|:
 // 1. Signature contained in the reply is valid.
 // 2. Certficate used to sign is rooted to a trusted CA.
 AuthResult AuthenticateChallengeReply(const CastMessage& challenge_reply,
                                       const std::string& peer_cert);
 
+// Sets trusted certificate authorities.
+bool SetTrustedCertificateAuthorities(const std::string& keys,
+                                      const std::string& signature);
+
 }  // namespace cast_channel
 }  // namespace core_api
 }  // namespace extensions
 
 #endif  // EXTENSIONS_BROWSER_API_CAST_CHANNEL_CAST_AUTH_UTIL_H_