Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
627573002:
Enable passing cast channel certificate authority keys. (Closed)
|
Created:
6 years, 2 months ago by vadimgo Modified:
6 years, 1 month ago Reviewers:
Ken Rockot(use gerrit already), Devlin, jar (doing other things), Ryan Sleevi, Finnur, mark a. foltz, dougsteed, Kevin M, not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionEnable passing cast channel certificate authority keys.
BUG=417090
Committed: https://crrev.com/df6b610eb8fb8cdaac5eb6b16aafefac3c90adb9
Cr-Commit-Position: refs/heads/master@{#302141}
Patch Set 1 #Patch Set 2 : Enable passing cast channel certificate authority keys. #Patch Set 3 : Added trusted certificate authorities data verification. #
Total comments: 8
Patch Set 4 : Code review changes. #Patch Set 5 : Updated idl. #Patch Set 6 : Fixed callback signature #Patch Set 7 : Use SHA-256 fingerprints and protobuf #Patch Set 8 : Removed unneeded header include. #
Total comments: 22
Patch Set 9 : Set trusted ICAs. #Patch Set 10 : Fixed initialization code. #
Total comments: 24
Patch Set 11 : Code review feedback changes. #
Total comments: 12
Patch Set 12 : Code review changes. #Patch Set 13 : Removed unused include. #Patch Set 14 : Added static to consts. #
Total comments: 18
Patch Set 15 : Code review changes #Patch Set 16 : Browser and unit tests. #
Total comments: 4
Patch Set 17 : Code review changes. #
Total comments: 48
Patch Set 18 : Code review changes. #
Total comments: 10
Patch Set 19 : Code review changes. #
Total comments: 15
Patch Set 20 : Code review changes. #Patch Set 21 : Added comments and rebased. #Patch Set 22 : Added SetTrustedCerificateAuthorities stub to cast_auth_util_openssl and rebased. #
Total comments: 2
Patch Set 23 : Synced with nss/ssl common code changes. #
Total comments: 2
Patch Set 24 : Updated histograms.xml #Patch Set 25 : Rebased. #Patch Set 26 : Added newline in cast_channel_apitest.cc. #Messages
Total messages: 77 (25 generated)
mfoltz@chromium.org changed reviewers: + mfoltz@chromium.org
I have concerns about this approach. (1) It creates an opportunity for an attacker to pass malformed key data to the browser and potentially compromise it. (2) It lowers the bar for an attacker to connect to non-authorized device, which sort of betrays the purpose of the certificates to begin with. This approach needs a design doc and sign-off from Chrome security before going forward. Please loop in rsleevi@ from Chrome and kmarshall@ from our team as they have context on this. https://codereview.chromium.org/627573002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:32: static const char kDefaultKeysData[] = { Serializing the keys into a continuous blob without delimiters assumes that all keys are the same format. We are pretty much SOL if the format of new keys changes. https://codereview.chromium.org/627573002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:646: const uint8 kSHA256WithRSAAlgorithmID[] = { Constants should be declared at the top level in the file. https://codereview.chromium.org/627573002/diff/40001/extensions/common/api/ca... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/40001/extensions/common/api/ca... extensions/common/api/cast_channel.idl:198: // Sets intermediate certificate authority keys. What is the format of the inputs? We need to add ~16 keys, how does one set multiple keys with two strings? Should this be addAuthorityKey(signature, key) instead? https://codereview.chromium.org/627573002/diff/40001/extensions/common/api/ca... extensions/common/api/cast_channel.idl:200: DOMString keys); This should have a callback argument (even one that takes no parameters) since the function is executed in a different process.
This approach pursues appoaches 1 and 2 proposed here: https://code.google.com/p/chromium/issues/detail?id=410668#c5 https://codereview.chromium.org/627573002/diff/40001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:32: static const char kDefaultKeysData[] = { The size of the fingerprint is constant at 20 bytes. There is a size field before the public key which allows supporting different format keys. https://codereview.chromium.org/627573002/diff/40001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:646: const uint8 kSHA256WithRSAAlgorithmID[] = { On 2014/10/03 20:07:12, mark a. foltz wrote: > Constants should be declared at the top level in the file. Done. https://codereview.chromium.org/627573002/diff/40001/extensions/common/api/ca... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/40001/extensions/common/api/ca... extensions/common/api/cast_channel.idl:198: // Sets intermediate certificate authority keys. Added a comment with the format, which is multiple records consisting of fingerprint (20 bytes), key size (2 bytes), key https://codereview.chromium.org/627573002/diff/40001/extensions/common/api/ca... extensions/common/api/cast_channel.idl:200: DOMString keys); On 2014/10/03 20:07:12, mark a. foltz wrote: > This should have a callback argument (even one that takes no parameters) since > the function is executed in a different process. Done.
vadimgo@chromium.org changed reviewers: + dougsteed@chromium.org, rsleevi@chromium.org
Thanks, the proto buffer is making me much happier with the patch. I'm sure there are a few things I'm missing on the crypto side, but here's what I have so far. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:617: std::vector<ICACertInfo> g_AllowedICAs; g_ICAPublicKeys https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:617: std::vector<ICACertInfo> g_AllowedICAs; Globals of class type are forbidden. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl... Please use base::LazyInstance instead. As a TODO, it would seem appropriate to refactor this module into a class since it is now managing state (the list of ICAs). https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:625: static void InitializeAllowedICAs() { InitializeICAPublicKeys() https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:626: for (size_t i = 0; i < arraysize(kAllowedICAs); i += 2) { Unless I'm misreading the kAllowedICAs definition, shouldn't this be i++? https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:633: static const SECItem* GetICAWithFingerprint( More specifically, GetICAPublicKeyWithFingerprint() https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:714: net::X509Certificate::CalculateChainFingerprint256( Have you verified that this computes the same fingerprints as is provided above? Also the script that will be used to generate the protobuf will need to changed as well. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:714: net::X509Certificate::CalculateChainFingerprint256( ISTM there should be a method on ica_cert to compute the SHA-256, i.e. ica_cert->fingerprint_sha256()... https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:891: return false; Don't we need to update g_AllowedICAs here? https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:529: if (cast_channel::SetTrustedCertificateAuthorities(signature, keys)) { |signature| and |keys| should be sanity checked and an error returned on invalid data. For example |signature| should be a deterministic size and keys.size() should be > 0 and < some reasonably large maximum. I worry about attempts to overflow the crypto routines. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:530: SetResult(new base::StringValue("")); // Intentionally returns a blank. Can this just fire an empty callback? https://codereview.chromium.org/627573002/diff/140001/extensions/common/api/c... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/140001/extensions/common/api/c... extensions/common/api/cast_channel.idl:201: // Sets trusted certificate authorities using multiple records in the Please update docstring to refer to the protobuf.
I've provided a preliminary scan, but there are a LOT of style issues. I'd like to ask you review http://google-styleguide.googlecode.com/svn/trunk/cppguide.html , in particular the areas I've highlighted as style below, and try to ensure this CL is consistent in it's application of it. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:63: const std::string& keys); nit: Logically, I tend to think of "keys" and then "signature" (as signature is over keys) https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:32: static const unsigned char kFingerprintICA1[] = { You changed the format here, without updating the document. Is this a SHA-256 hash now? If so, net::SHA256HashValue https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:618: std::unique_ptr<proto::AuthorityKeys> g_AuthorityKeys; BUG: We do not allow static class-types STYLE: g_AuthorityKeys -> g_authority_keys https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:704: } BUG: Thread-unsafe initialization https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:794: const uint8 kPublicKey[294]={ STYLE: "const uint8 kPublicKey[] = { " 1) No need to list explicit size 2) Spaces in initialization https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:824: { STYLE: Brace goes on the former line. You can use "git cl format" to fix many of these style issues. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:880: std::string decodedSignature; STYLE: These variables need to be named consistent with Chromium / Google style decoded_signature decoded_keys authority_keys etc https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:891: std::unique_ptr<proto::AuthorityKeys> authorityKeys(new proto::AuthorityKeys); STYLE: use scoped_ptr<>. unique_ptr<> is NOT blessed https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:893: return false; STYLE: Invalid indent https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:903: { STYLE: This is some crazy syntax (isn't it GCC dependent?) Write it clearer, such as by explicitly creating and initializing a struct and then using .push_back() of that struct (which is cheap and effectively the same op codes) https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:907: (unsigned char*)public_key.data(), style: Invalid cast (use static_cast) https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:530: SetResult(new base::StringValue("")); // Intentionally returns a blank. std::string(), not "" https://codereview.chromium.org/627573002/diff/220001/extensions/common/api/c... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/220001/extensions/common/api/c... extensions/common/api/cast_channel.idl:203: // fingerprint (20 bytes), public key size (2 bytes), public key documentation is out of date
https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:617: std::vector<ICACertInfo> g_AllowedICAs; On 2014/10/09 17:57:20, mark a. foltz wrote: > Globals of class type are forbidden. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl... > > Please use base::LazyInstance instead. > > As a TODO, it would seem appropriate to refactor this module into a class since > it is now managing state (the list of ICAs). Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:617: std::vector<ICACertInfo> g_AllowedICAs; On 2014/10/09 17:57:20, mark a. foltz wrote: > g_ICAPublicKeys Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:617: std::vector<ICACertInfo> g_AllowedICAs; On 2014/10/09 17:57:20, mark a. foltz wrote: > g_ICAPublicKeys Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:626: for (size_t i = 0; i < arraysize(kAllowedICAs); i += 2) { On 2014/10/09 17:57:19, mark a. foltz wrote: > Unless I'm misreading the kAllowedICAs definition, shouldn't this be i++? Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:633: static const SECItem* GetICAWithFingerprint( On 2014/10/09 17:57:20, mark a. foltz wrote: > More specifically, GetICAPublicKeyWithFingerprint() Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:714: net::X509Certificate::CalculateChainFingerprint256( On 2014/10/09 17:57:19, mark a. foltz wrote: > Have you verified that this computes the same fingerprints as is provided above? > Also the script that will be used to generate the protobuf will need to changed > as well. Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:714: net::X509Certificate::CalculateChainFingerprint256( On 2014/10/09 17:57:20, mark a. foltz wrote: > ISTM there should be a method on ica_cert to compute the SHA-256, i.e. > ica_cert->fingerprint_sha256()... Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:891: return false; On 2014/10/09 17:57:19, mark a. foltz wrote: > Don't we need to update g_AllowedICAs here? Done. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:529: if (cast_channel::SetTrustedCertificateAuthorities(signature, keys)) { Added checks here and in SetTrustedCertificateAuthorities. On 2014/10/09 17:57:20, mark a. foltz wrote: > |signature| and |keys| should be sanity checked and an error returned on invalid > data. For example |signature| should be a deterministic size and keys.size() > should be > 0 and < some reasonably large maximum. I worry about attempts to > overflow the crypto routines. https://codereview.chromium.org/627573002/diff/140001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:530: SetResult(new base::StringValue("")); // Intentionally returns a blank. I couldn't figure out how to do it, it seems to require a value. On 2014/10/09 17:57:20, mark a. foltz wrote: > Can this just fire an empty callback? https://codereview.chromium.org/627573002/diff/140001/extensions/common/api/c... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/140001/extensions/common/api/c... extensions/common/api/cast_channel.idl:201: // Sets trusted certificate authorities using multiple records in the On 2014/10/09 17:57:20, mark a. foltz wrote: > Please update docstring to refer to the protobuf. Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:63: const std::string& keys); I will make this change if you insist, but if I can get away with it, at this point there are many places I'd need to touch. On 2014/10/09 21:02:42, Ryan Sleevi wrote: > nit: Logically, I tend to think of "keys" and then "signature" (as signature is > over keys) https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:32: static const unsigned char kFingerprintICA1[] = { I did update the document. I would prefer to keep both fingerprint and public key as "const unsigned int" and fill them into cert info below. On 2014/10/09 21:02:42, Ryan Sleevi wrote: > You changed the format here, without updating the document. Is this a SHA-256 > hash now? If so, net::SHA256HashValue https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:618: std::unique_ptr<proto::AuthorityKeys> g_AuthorityKeys; On 2014/10/09 21:02:42, Ryan Sleevi wrote: > BUG: We do not allow static class-types > STYLE: g_AuthorityKeys -> g_authority_keys Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:704: } On 2014/10/09 21:02:42, Ryan Sleevi wrote: > BUG: Thread-unsafe initialization Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:794: const uint8 kPublicKey[294]={ On 2014/10/09 21:02:43, Ryan Sleevi wrote: > STYLE: "const uint8 kPublicKey[] = { " > > 1) No need to list explicit size > 2) Spaces in initialization Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:824: { On 2014/10/09 21:02:42, Ryan Sleevi wrote: > STYLE: Brace goes on the former line. > > You can use "git cl format" to fix many of these style issues. Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:880: std::string decodedSignature; On 2014/10/09 21:02:42, Ryan Sleevi wrote: > STYLE: These variables need to be named consistent with Chromium / Google style > > decoded_signature > decoded_keys > authority_keys > etc Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:891: std::unique_ptr<proto::AuthorityKeys> authorityKeys(new proto::AuthorityKeys); On 2014/10/09 21:02:42, Ryan Sleevi wrote: > STYLE: use scoped_ptr<>. unique_ptr<> is NOT blessed Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:893: return false; On 2014/10/09 21:02:43, Ryan Sleevi wrote: > STYLE: Invalid indent Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:903: { On 2014/10/09 21:02:43, Ryan Sleevi wrote: > STYLE: This is some crazy syntax (isn't it GCC dependent?) > > Write it clearer, such as by explicitly creating and initializing a struct and > then using .push_back() of that struct (which is cheap and effectively the same > op codes) Done. https://codereview.chromium.org/627573002/diff/220001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:907: (unsigned char*)public_key.data(), On 2014/10/09 21:02:42, Ryan Sleevi wrote: > style: Invalid cast (use static_cast) Done.
https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:63: const std::string& keys); Reminder on the ordering https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:34: static const unsigned char kFingerprintICA1[] = { You didn't fix this https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:613: } So this is still full of the thread-safe anti-patterns Rather than three lazy instances, you make a single class (including constructor that does initializing), and then make that a leaky lazy instance. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:672: base::AutoLock auto_lock(g_authority_keys_lock.Get()); Locks are an anti-pattern for dealing with thread-safety. 1) You should ideally avoid locks with proper lazy initialization 2) Even if you were going to lock (for example, because the authorities may change), you should use lock-and-copy, not lock for the entire method 3) Are locks entirely appropriate? Wouldn't it be better if the authorities were a member of the API calls / state? That is, one profile adding additional authorities wouldn't affect another profile (or extension) https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:896: public_key.size()}}; SECURITY BUG: You're assuming that fingerprint.data (which is just a sequence of bytes) will be the proper hash algorithm and size. That's a recipe for danger. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:530: if (signature.size() > 0 && keys.size() > 0 && When using the STL, prefer .empty() instead of .size() > 0 For some containers, it's vastly more efficient, so it's good to be idiomatic.
Ryan, I added a helper class for storing cast channel intermediate certificate authorities, changed parameter ordering as you suggested, and updated fingerprint const initializers. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:63: const std::string& keys); On 2014/10/11 02:22:36, Ryan Sleevi wrote: > Reminder on the ordering Done. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:34: static const unsigned char kFingerprintICA1[] = { On 2014/10/11 02:22:36, Ryan Sleevi wrote: > You didn't fix this Done. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:613: } On 2014/10/11 02:22:36, Ryan Sleevi wrote: > So this is still full of the thread-safe anti-patterns > > Rather than three lazy instances, you make a single class (including constructor > that does initializing), and then make that a leaky lazy instance. Done. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:672: base::AutoLock auto_lock(g_authority_keys_lock.Get()); On 2014/10/11 02:22:36, Ryan Sleevi wrote: > Locks are an anti-pattern for dealing with thread-safety. > > 1) You should ideally avoid locks with proper lazy initialization > 2) Even if you were going to lock (for example, because the authorities may > change), you should use lock-and-copy, not lock for the entire method > 3) Are locks entirely appropriate? Wouldn't it be better if the authorities were > a member of the API calls / state? That is, one profile adding additional > authorities wouldn't affect another profile (or extension) Done. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:896: public_key.size()}}; On 2014/10/11 02:22:36, Ryan Sleevi wrote: > SECURITY BUG: You're assuming that fingerprint.data (which is just a sequence of > bytes) will be the proper hash algorithm and size. That's a recipe for danger. Added a check for the size of fingerprint. https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/290001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:530: if (signature.size() > 0 && keys.size() > 0 && On 2014/10/11 02:22:36, Ryan Sleevi wrote: > When using the STL, prefer .empty() instead of .size() > 0 > > For some containers, it's vastly more efficient, so it's good to be idiomatic. Done.
Getting closer! - The design of this means that there is one singleton key store shared by all extensions. If there are multiple extensions with different keystores whoever loads keys last will seem to win. This could be extended to support multiple per-extension keystores, e.g. by declaring the AuthKeyStore as an ApiResource, but I expect this to be a corner case so not worth the complexity. - cast_auth_util.cc and cast_auth_util_nss.cc should have unit test cases that exercise the loading of and use the keystore. - The new API function should have a browser test written that loads a single key and verifies success, and loads a bogus key and verifies lastError is set. See cast_channel_apitest.cc and the support files in chrome/test/data/extensions/api_test/cast_channel/api. Ping me if you need any help figuring out the extension test framework. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:630: // Returns NULL, if no such ICA is found. This documentation belongs in the .h https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:643: // Returns the public key of the first ICA in the list. Document behavior of returning NULL on empty list; and move documentation to the .h https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:672: certificate_authorities_.push_back(cert_info); Is there any way to validate that public_key is, in fact, a valid certificate? And that the certificate fingerprint matches? If we push a bad certificate it could be slightly more difficult to diagnose the problem - we may notice that all connection attempts fail to a certain device, but if it is not commonly used we might miss it. Any extra validation we do here can be used to signal the extension and log a message to be included in user feedback. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:64: const SECItem* GetICAPublicKeyFromFingerprint( Please document these method declarations. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:78: static const ICACertInfo kAllowedICAs[]; This holds the existing hard coded values, right? Please document this to distinguish it from certificate_authorities_. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:84: proto::AuthorityKeys authority_keys_; Why does the proto::AuthorityKeys have to be retained beyond the call to Load()? https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:532: SetResult(new base::StringValue(std::string())); What does the blank string signify? If you just need to signal success by firing an empty callback, you can invoke AsyncWorkCompleted(). https://codereview.chromium.org/627573002/diff/680001/extensions/common/api/c... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/680001/extensions/common/api/c... extensions/common/api/cast_channel.idl:162: callback SetAuthorityKeysCallback = void (DOMString result); Why can't this be void ()? If we need to add an argument to the callback later that will be backwards compatible.
https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:672: certificate_authorities_.push_back(cert_info); On 2014/10/14 06:15:04, mark a. foltz wrote: > Is there any way to validate that public_key is, in fact, a valid certificate? > And that the certificate fingerprint matches? > > If we push a bad certificate it could be slightly more difficult to diagnose the > problem - we may notice that all connection attempts fail to a certain device, > but if it is not commonly used we might miss it. Any extra validation we do > here can be used to signal the extension and log a message to be included in > user feedback. We don't have the certificates here, only the fingerprints and public keys. The data structure is generated offline from the certificates. So no we can't check that here unfortunately. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:184: const uint8 kPublicKey[] = { This is not the correct public key, presumably it's here for test purposes and will be replaced by the correct one before checkin?
https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:630: // Returns NULL, if no such ICA is found. On 2014/10/14 06:15:04, mark a. foltz wrote: > This documentation belongs in the .h Done. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:643: // Returns the public key of the first ICA in the list. On 2014/10/14 06:15:04, mark a. foltz wrote: > Document behavior of returning NULL on empty list; and move documentation to the > .h Done. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:64: const SECItem* GetICAPublicKeyFromFingerprint( On 2014/10/14 06:15:04, mark a. foltz wrote: > Please document these method declarations. Done. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:78: static const ICACertInfo kAllowedICAs[]; On 2014/10/14 06:15:04, mark a. foltz wrote: > This holds the existing hard coded values, right? Please document this to > distinguish it from certificate_authorities_. Done. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:84: proto::AuthorityKeys authority_keys_; On 2014/10/14 06:15:04, mark a. foltz wrote: > Why does the proto::AuthorityKeys have to be retained beyond the call to Load()? certificate_authorities_ points to either hard-coded const or protobuf data - that's why protobuf data is retained. I added a comment on this. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:184: const uint8 kPublicKey[] = { On 2014/10/14 18:09:51, dougsteed wrote: > This is not the correct public key, presumably it's here for test purposes and > will be replaced by the correct one before checkin? That's correct - once the proper key is available, it will replace this. https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/627573002/diff/680001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:532: SetResult(new base::StringValue(std::string())); On 2014/10/14 06:15:04, mark a. foltz wrote: > What does the blank string signify? If you just need to signal success by > firing an empty callback, you can invoke AsyncWorkCompleted(). Removed the result. https://codereview.chromium.org/627573002/diff/680001/extensions/common/api/c... File extensions/common/api/cast_channel.idl (right): https://codereview.chromium.org/627573002/diff/680001/extensions/common/api/c... extensions/common/api/cast_channel.idl:162: callback SetAuthorityKeysCallback = void (DOMString result); On 2014/10/14 06:15:04, mark a. foltz wrote: > Why can't this be void ()? If we need to add an argument to the callback later > that will be backwards compatible. Done.
Patchset #18 (id:760001) has been deleted
Patchset #17 (id:740001) has been deleted
Patchset #16 (id:720001) has been deleted
Added browser and unit tests.
On 2014/10/15 19:45:32, vadimgo wrote: > Added browser and unit tests. lgtm % a couple of remaining comments. Please wait for rsleevi@'s lgtm as well. Do you plan on waiting for the proper public key before landing?
On 2014/10/16 22:15:33, mark a. foltz wrote: > On 2014/10/15 19:45:32, vadimgo wrote: > > Added browser and unit tests. > > lgtm % a couple of remaining comments. Please wait for rsleevi@'s lgtm as well. > Do you plan on waiting for the proper public key before landing? Yes, I will wait for the proper key - we should have it early next week.
Somehow my last message did not include them... https://codereview.chromium.org/627573002/diff/780001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right): https://codereview.chromium.org/627573002/diff/780001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:35: void CastChannelAuthorityKeysTest::TestKeys() { ExpectDefaultKeysLoaded() - since TestKeys makes it sound like a standalone test case. https://codereview.chromium.org/627573002/diff/780001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:72: TestKeys(); Can you update the test to verify than an additional key loaded from the protobuf is also available?
https://codereview.chromium.org/627573002/diff/780001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right): https://codereview.chromium.org/627573002/diff/780001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:35: void CastChannelAuthorityKeysTest::TestKeys() { On 2014/10/16 22:46:18, mark a. foltz wrote: > ExpectDefaultKeysLoaded() - since TestKeys makes it sound like a standalone test > case. Done. Renamed it to ExpectKeysLoaded. https://codereview.chromium.org/627573002/diff/780001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:72: TestKeys(); On 2014/10/16 22:46:18, mark a. foltz wrote: > Can you update the test to verify than an additional key loaded from the > protobuf is also available? Done.
There's still a lot of style issues here. Apologies for marking it up so much, but hopefully you will appreciate the thoroughness. Apologies for the delay - been out with the flu. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:38: // Fingerprints and public keys of the allowed / trusted ICAs. Seems like these may be better off moved to a separate file, just to make it easier to read/maintain? Alternatively, you should move these all to an unnamed namespace at the top of the file. That is, favour readability - having Authresult (line 30-37) broken up by this large chunk of statics can make it harder to match the header to the code and to follow the logical flow. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:616: sizeof(kPublicKeyICA16)}}, Did git-cl format do this? The indents look off and inconsistent. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:658: if (fingerprint.size() != 32) { SAFETY: if (fingerprint.size() != arraysize(ICACertInfo.fingerprint.data)) https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:667: public_key.size()}}; STYLE: Compound initializer lists are still discouraged in Chromium. While not to be confused with C+11 initializer lists, or C11 named initializer lists, this is still an unorthodox style. Further, you have failed to correct the style error I pointed out before - needing to static_cast<> this. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:8: #include <string> STYLE: Newline between lines 8 and 9 https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:9: #include "crypto/scoped_nss_types.h" 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 https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:10: #include "extensions/common/api/cast_channel/authority_keys.pb.h" 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) https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:58: // Helper class for intermediate certificate authority validation. 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. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:71: const SECItem* GetDefaultICAPublicKey(); 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. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:74: bool Load(std::string& keys); 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? https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:77: // Info for trusted ICA certs. 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) https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:85: std::vector<ICACertInfo> certificate_authorities_; 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> ? https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:88: static const ICACertInfo kAllowedICAs[]; 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. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:92: }; STYLE: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:98: if (num_intermediates <= 0) { BUG? when can num_intermediates every be < 0? Isn't that a protocol bug? https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:267: bool SetTrustedCertificateAuthorities(const std::string& keys, This can only ever be called on one thread, correct? Otherwise, there are *still* threading issues. You're now safely initializing g_trusted_authorities, but you're still allowing g_trusted_authorities.whatever_that_member_is_ to be mutated by concurrent calls to Load (line 283) Your threading guarantees are unclear and undocumented, AFAICT, so that's why I'm asking. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:277: if (decoded_signature.size() != 256) I don't see why this check is necessary, but it further couples this function to the implementation of VerifySignature (which has the public key) I would remove this check or, if necessary, it should be part of VerifySignature's consistency check, since it's a property of the public key. (To be clear, I don't think you should check it) https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:6: #include "base/base64.h" STYLE: newline between lines 5 and 6 https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:7: #include "extensions/browser/api/cast_channel/cast_auth_util.h" STYLE: Per http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... , unittest files are treated the same as their non-unittest equivalent That is, this header (cast_auth_util.h) should be first, followed by the system headers, etc https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:13: class CastChannelAuthorityKeysTest : public testing::Test { STYLE: Newline between lines 10, 11, 12, 13 https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:46: authority_keys_store_.GetICAPublicKeyFromFingerprint(kFingerprintValid1); did git gl format do this? Doesn't seem right. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:51: EXPECT_EQ(static_cast<SECItem*>(NULL), key); This is a non-_nss file, you should not be using NSS types. https://codereview.chromium.org/627573002/diff/800001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/627573002/diff/800001/extensions/extensions.g... extensions/extensions.gyp:1056: "../crypto/crypto.gyp:crypto", After you fix the design / IWYU / _nss file issues I point out, this should no longer be necessary. https://codereview.chromium.org/627573002/diff/800001/net/cert/x509_certifica... File net/cert/x509_certificate.h (right): https://codereview.chromium.org/627573002/diff/800001/net/cert/x509_certifica... net/cert/x509_certificate.h:224: } STYLE: Don't inline this. It violates the trivial inline rule. DESIGN: NACK on this. It should be a static method ala CalculateFingerprint that takes a single OSCertHandle. CalculateFingerprint256 (That said, this is being added in https://codereview.chromium.org/547603002/diff/260001/net/cert/x509_certifica... , but that CL may take a bit longer to review. You should reach out to Eran and split off just that portion of his CL into a **NEW** CL, then land that, then make this CL depend on that)
Patchset #18 (id:820001) has been deleted
Ryan, thanks for your review. Please take another look. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:38: // Fingerprints and public keys of the allowed / trusted ICAs. On 2014/10/17 19:53:20, Ryan Sleevi wrote: > Seems like these may be better off moved to a separate file, just to make it > easier to read/maintain? Alternatively, you should move these all to an unnamed > namespace at the top of the file. > > That is, favour readability - having Authresult (line 30-37) broken up by this > large chunk of statics can make it harder to match the header to the code and to > follow the logical flow. Moved these to the top of the file in an unnamed namespace. These should go away, once the cast extension calls the new api. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:616: sizeof(kPublicKeyICA16)}}, On 2014/10/17 19:53:20, Ryan Sleevi wrote: > Did git-cl format do this? The indents look off and inconsistent. Yes "git cl format" formatted this. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:658: if (fingerprint.size() != 32) { On 2014/10/17 19:53:20, Ryan Sleevi wrote: > SAFETY: if (fingerprint.size() != arraysize(ICACertInfo.fingerprint.data)) Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:667: public_key.size()}}; On 2014/10/17 19:53:20, Ryan Sleevi wrote: > STYLE: Compound initializer lists are still discouraged in Chromium. While not > to be confused with C+11 initializer lists, or C11 named initializer lists, this > is still an unorthodox style. > > Further, you have failed to correct the style error I pointed out before - > needing to static_cast<> this. Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:8: #include <string> On 2014/10/17 19:53:20, Ryan Sleevi wrote: > STYLE: Newline between lines 8 and 9 Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:9: #include "crypto/scoped_nss_types.h" On 2014/10/17 19:53:20, Ryan Sleevi wrote: > 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 Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:10: #include "extensions/common/api/cast_channel/authority_keys.pb.h" On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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) Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:58: // Helper class for intermediate certificate authority validation. On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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. > Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:71: const SECItem* GetDefaultICAPublicKey(); On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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. Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:74: bool Load(std::string& keys); On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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? Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:77: // Info for trusted ICA certs. On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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) Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:85: std::vector<ICACertInfo> certificate_authorities_; On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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> > ? Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:88: static const ICACertInfo kAllowedICAs[]; On 2014/10/17 19:53:21, Ryan Sleevi wrote: > 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. Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:92: }; On 2014/10/17 19:53:21, Ryan Sleevi wrote: > STYLE: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:98: if (num_intermediates <= 0) { On 2014/10/17 19:53:21, Ryan Sleevi wrote: > BUG? when can num_intermediates every be < 0? Isn't that a protocol bug? Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:267: bool SetTrustedCertificateAuthorities(const std::string& keys, On 2014/10/17 19:53:21, Ryan Sleevi wrote: > This can only ever be called on one thread, correct? > > Otherwise, there are *still* threading issues. You're now safely initializing > g_trusted_authorities, but you're still allowing > g_trusted_authorities.whatever_that_member_is_ to be mutated by concurrent calls > to Load (line 283) > > Your threading guarantees are unclear and undocumented, AFAICT, so that's why > I'm asking. There are two assumptions 1) The calls should happen on one thread from Javascript code. 2) Only the cast extension Javascript code should be able to call this api via validation of the authorities information signature. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:277: if (decoded_signature.size() != 256) On 2014/10/17 19:53:21, Ryan Sleevi wrote: > I don't see why this check is necessary, but it further couples this function to > the implementation of VerifySignature (which has the public key) > > I would remove this check or, if necessary, it should be part of > VerifySignature's consistency check, since it's a property of the public key. > (To be clear, I don't think you should check it) Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right): https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:6: #include "base/base64.h" On 2014/10/17 19:53:21, Ryan Sleevi wrote: > STYLE: newline between lines 5 and 6 Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:7: #include "extensions/browser/api/cast_channel/cast_auth_util.h" On 2014/10/17 19:53:21, Ryan Sleevi wrote: > STYLE: Per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... > , unittest files are treated the same as their non-unittest equivalent > > That is, this header (cast_auth_util.h) should be first, followed by the system > headers, etc Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:13: class CastChannelAuthorityKeysTest : public testing::Test { On 2014/10/17 19:53:21, Ryan Sleevi wrote: > STYLE: Newline between lines 10, 11, 12, 13 Done. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:46: authority_keys_store_.GetICAPublicKeyFromFingerprint(kFingerprintValid1); On 2014/10/17 19:53:21, Ryan Sleevi wrote: > did git gl format do this? Doesn't seem right. Yes, "git cl format" formatted this. https://codereview.chromium.org/627573002/diff/800001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:51: EXPECT_EQ(static_cast<SECItem*>(NULL), key); On 2014/10/17 19:53:21, Ryan Sleevi wrote: > This is a non-_nss file, you should not be using NSS types. Done. https://codereview.chromium.org/627573002/diff/800001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/627573002/diff/800001/extensions/extensions.g... extensions/extensions.gyp:1056: "../crypto/crypto.gyp:crypto", On 2014/10/17 19:53:21, Ryan Sleevi wrote: > After you fix the design / IWYU / _nss file issues I point out, this should no > longer be necessary. Done. https://codereview.chromium.org/627573002/diff/800001/net/cert/x509_certifica... File net/cert/x509_certificate.h (right): https://codereview.chromium.org/627573002/diff/800001/net/cert/x509_certifica... net/cert/x509_certificate.h:224: } On 2014/10/17 19:53:21, Ryan Sleevi wrote: > STYLE: Don't inline this. It violates the trivial inline rule. > DESIGN: NACK on this. It should be a static method ala CalculateFingerprint that > takes a single OSCertHandle. CalculateFingerprint256 > > (That said, this is being added in > https://codereview.chromium.org/547603002/diff/260001/net/cert/x509_certifica... > , but that CL may take a bit longer to review. You should reach out to Eran and > split off just that portion of his CL into a **NEW** CL, then land that, then > make this CL depend on that) Moved from incline to code file and contacted Eran to see if his change can be split off.
Still lgtm https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:16: static const net::SHA256HashValue kFingerprintICA1 = {{0x52, What is up with this formatting - can it be rewrapped to 8 or 12 digits/line like below?
https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:1028: return false; If this fails, then all cast connections will fail until Chrome is restarted (because the initial value of certificate_authorities_ is never reset) Is this what you want? https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:1034: if (fingerprint.size() != sizeof(net::SHA256HashValue)) { 1) Don't do sizeof the struct (padding, etc). You should be able to do sizeof(ICACertInfo.fingerprint->data), right? https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:68: } } // namespace proto (with newline between 67/68 https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:82: const base::StringPiece* GetICAPublicKeyFromFingerprint( Why are you returning a pointer to this? This creates danger if someone does Load() while such a pointer is still hanging out there. You can instead just return a base::StringPiece - no pointer.
Thank you, Mark and Ryan. https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:16: static const net::SHA256HashValue kFingerprintICA1 = {{0x52, On 2014/10/21 20:22:14, mark a. foltz wrote: > What is up with this formatting - can it be rewrapped to 8 or 12 digits/line > like below? Unfortunately "git cl format" does this. I changed to the original format generated by the tool. https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:1028: return false; On 2014/10/21 22:38:03, Ryan Sleevi wrote: > If this fails, then all cast connections will fail until Chrome is restarted > (because the initial value of certificate_authorities_ is never reset) > > Is this what you want? Our intention is to remove the hard-coded ICA data from Chrome altogether and rely purely on the extension to pass it in. So yes, this is intended. https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:1034: if (fingerprint.size() != sizeof(net::SHA256HashValue)) { On 2014/10/21 22:38:03, Ryan Sleevi wrote: > 1) Don't do sizeof the struct (padding, etc). You should be able to do > sizeof(ICACertInfo.fingerprint->data), right? I changed to this: if (fingerprint.size() != sizeof(((net::SHA256HashValue*)0)->data)) I don't want to rely on ICACertInfo, as that should be removed when the hard-coded data is removed. https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:68: } On 2014/10/21 22:38:03, Ryan Sleevi wrote: > } // namespace proto (with newline between 67/68 Done. https://codereview.chromium.org/627573002/diff/840001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:82: const base::StringPiece* GetICAPublicKeyFromFingerprint( On 2014/10/21 22:38:03, Ryan Sleevi wrote: > Why are you returning a pointer to this? This creates danger if someone does > Load() while such a pointer is still hanging out there. You can instead just > return a base::StringPiece - no pointer. Done.
LGTM, but there are several things to be addressed before committing. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:624: const_cast<char*>(fingerprint.data())); BUG: This isn't safe casting (from char to a SHA256HashValue). Let's just try to be clear, rather than clever :) https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:627: *hash, base::StringPiece(public_key.data(), public_key.size()))); base::StringPiece is implicitly constructible from std::string. You shouldn't need to create from .data()/.size() here https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:69: } // namespace proto Realized this isn't right anymore. This should move to line 14/15 (eg: outside of extensions::core_api::cast_channel) https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:76: class AuthorityKeysStore { naming wise, does this make sense singular? AuthorityKeyStore? https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:90: // Loads authority keys from a serialized protobuf. Documentation: This invalidates all previously returned Public Keys, right? Worth documenting. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:288: return g_authority_keys_store.Get().Load(decoded_keys); You don't seem to have any tests for this function, do you? Tests to consider: Invalid base-64 keys invalid base-64 signature invalid signature truncated signature https://codereview.chromium.org/627573002/diff/860001/net/cert/x509_certifica... File net/cert/x509_certificate.h (right): https://codereview.chromium.org/627573002/diff/860001/net/cert/x509_certifica... net/cert/x509_certificate.h:221: const SHA256HashValue fingerprint256() const; Remove this now that the CalculateFingerprint256(OSCertHandle()) stuff is in master
Patchset #20 (id:880001) has been deleted
Please note that in addition to code review changes, the latest patch also contains the final verification public key generated by Doug. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:624: const_cast<char*>(fingerprint.data())); On 2014/10/22 22:07:09, Ryan Sleevi wrote: > BUG: This isn't safe casting (from char to a SHA256HashValue). > > Let's just try to be clear, rather than clever :) Done. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.cc:627: *hash, base::StringPiece(public_key.data(), public_key.size()))); On 2014/10/22 22:07:10, Ryan Sleevi wrote: > base::StringPiece is implicitly constructible from std::string. You shouldn't > need to create from .data()/.size() here Done. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:69: } // namespace proto On 2014/10/22 22:07:10, Ryan Sleevi wrote: > Realized this isn't right anymore. > > This should move to line 14/15 (eg: outside of > extensions::core_api::cast_channel) This is right, because the protobuf is also namespaced to extensions::core_api::cast_channel. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:76: class AuthorityKeysStore { On 2014/10/22 22:07:10, Ryan Sleevi wrote: > naming wise, does this make sense singular? AuthorityKeyStore? Done. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:90: // Loads authority keys from a serialized protobuf. On 2014/10/22 22:07:10, Ryan Sleevi wrote: > Documentation: This invalidates all previously returned Public Keys, right? > Worth documenting. Done. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:288: return g_authority_keys_store.Get().Load(decoded_keys); On 2014/10/22 22:07:10, Ryan Sleevi wrote: > You don't seem to have any tests for this function, do you? > > Tests to consider: > Invalid base-64 keys > invalid base-64 signature > invalid signature > truncated signature The tests are in extensions/browser/api/cast_channel/cast_channel_api.cc.
Still lgtm. Looks like you may need to rebase the patch before committing.
I'm going to LGTM this, but I still have reservations about the quality of the comments and tests. Please see below. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util.h (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util.h:90: // Loads authority keys from a serialized protobuf. On 2014/10/23 18:03:25, vadimgo wrote: > On 2014/10/22 22:07:10, Ryan Sleevi wrote: > > Documentation: This invalidates all previously returned Public Keys, right? > > Worth documenting. > > Done. Not really. It doesn't document the 'surprising' part of the API that I mentioned. // Loads authority keys from a serialized protobuf, returning true if the keys were successfully loaded. // Regardless of success or failure, this will invalidate // any keys previously returned (e.g. via GetICAPublicKeyFromFingerprint() // or GetDefaultICAPublicKey()) Alternatively, you would document for the Get functions (e.g. GetDefaultICAPublicKey()) // Returns the public key of the default / original // Cast ICA. Returns an empty StringPiece if the default // Cast ICA is not found. // Note: The returned StringPiece is invalidated if // Load() is called. https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/627573002/diff/860001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_nss.cc:288: return g_authority_keys_store.Get().Load(decoded_keys); On 2014/10/23 18:03:26, vadimgo wrote: > On 2014/10/22 22:07:10, Ryan Sleevi wrote: > > You don't seem to have any tests for this function, do you? > > > > Tests to consider: > > Invalid base-64 keys > > invalid base-64 signature > > invalid signature > > truncated signature > > The tests are in extensions/browser/api/cast_channel/cast_channel_api.cc. To be explicit: I was asking that you add explicit unit tests, not integration tests. You should be testing the function closest to this code - eg in util_unittest.cc
The CQ bit was checked by vadimgo@chromium.org
Updated the comments based on Ryan's suggestions and rebased.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/627573002/920001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
vadimgo@chromium.org changed reviewers: + finnur@chromium.org, kalman@chromium.org
On 2014/10/24 22:03:38, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Looks like the openssl impl is missing SetTrustedCertificateAuthorities?
rockot@chromium.org changed reviewers: + rockot@chromium.org
On 2014/10/25 14:47:14, Ken Rockot wrote: > On 2014/10/24 22:03:38, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Looks like the openssl impl is missing SetTrustedCertificateAuthorities? I'm referring to the linker errors seen on Mac builds, which presumably are using cast_auth_util_openssl (which does not seem to define SetTrustedCerificateAuthorities) instead of cast_auth_util_nss. My interpretation may be incorrect.
vadimgo@chromium.org changed reviewers: + jar@chromium.org
Added SetTrustedCertificateAuthorities stub to cast_auth_util_openssl and rebased.
https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right): https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:21: NOTREACHED(); Why is this not reached? Clearly it's being linked, and I don't see any platform restrictions on setAuthorityKeys.
https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right): https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:21: NOTREACHED(); On 2014/10/27 17:17:50, Ken Rockot wrote: > Why is this not reached? Clearly it's being linked, and I don't see any platform > restrictions on setAuthorityKeys. Talking to Doug, only the nss version is used for build flavors that cast cares about and openssl is only implemented to make other flavors link. SetTrustedCertificateAuthorities implementation is consistent with AuthenticateChallengeReply which are used to support the same functionality - cast channel authentication.
On 2014/10/27 17:29:54, vadimgo wrote: > https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... > File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right): > > https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... > extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:21: NOTREACHED(); > On 2014/10/27 17:17:50, Ken Rockot wrote: > > Why is this not reached? Clearly it's being linked, and I don't see any > platform > > restrictions on setAuthorityKeys. > > Talking to Doug, only the nss version is used for build flavors that cast cares > about and openssl is only implemented to make other flavors link. > SetTrustedCertificateAuthorities implementation is consistent with > AuthenticateChallengeReply which are used to support the same functionality - > cast channel authentication. OK, thanks. I guess ideally we just wouldn't build it on unsupported platforms, but it sounds like this is already happening, so LGTM.
The CQ bit was checked by vadimgo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/627573002/940001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/10/27 17:29:54, vadimgo wrote: > https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... > File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right): > > https://codereview.chromium.org/627573002/diff/940001/extensions/browser/api/... > extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:21: NOTREACHED(); > On 2014/10/27 17:17:50, Ken Rockot wrote: > > Why is this not reached? Clearly it's being linked, and I don't see any > platform > > restrictions on setAuthorityKeys. > > Talking to Doug, only the nss version is used for build flavors that cast cares > about and openssl is only implemented to make other flavors link. > SetTrustedCertificateAuthorities implementation is consistent with > AuthenticateChallengeReply which are used to support the same functionality - > cast channel authentication. You don't care about OS X? If so, this is news to me.
Removing LG until the OS X issues are addressed. You need to support OpenSSL, and I'm sorry for not pushing sooner. Not LGTM
Patchset #25 (id:990001) has been deleted
Patchset #24 (id:980001) has been deleted
Patchset #23 (id:960001) has been deleted
vadimgo@chromium.org changed reviewers: + kmarshall@chromium.org, rdevlin.cronin@chromium.org
Integrated with Kevin's nss/ssl agnostic ica changes.
LGTM once Kevin's stuff lands.
My CL has landed. On Wed, Oct 29, 2014 at 4:21 PM, <rsleevi@chromium.org> wrote: > LGTM once Kevin's stuff lands. > > https://codereview.chromium.org/627573002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
FWIW: I was looked around to see how populated the histogram called Extensions.FunctionCalls was... and found no data in it. You might want to see if some of this is dead code (or perhaps I was looking in the wrong place). https://codereview.chromium.org/627573002/diff/1010001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/627573002/diff/1010001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:971: CAST_CHANNEL_SETAUTHORITYKEYS, Please include the corresponding update to histograms.xml with this CL.
https://codereview.chromium.org/627573002/diff/1010001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/627573002/diff/1010001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:971: CAST_CHANNEL_SETAUTHORITYKEYS, On 2014/10/29 23:56:26, jar wrote: > Please include the corresponding update to histograms.xml with this CL. Updated histograms.xml.
On 2014/10/29 23:56:26, jar wrote: > FWIW: I was looked around to see how populated the histogram called > Extensions.FunctionCalls was... and found no data in it. You might want to see > if some of this is dead code (or perhaps I was looking in the wrong place). > > https://codereview.chromium.org/627573002/diff/1010001/extensions/browser/ext... > File extensions/browser/extension_function_histogram_value.h (right): > > https://codereview.chromium.org/627573002/diff/1010001/extensions/browser/ext... > extensions/browser/extension_function_histogram_value.h:971: > CAST_CHANNEL_SETAUTHORITYKEYS, > Please include the corresponding update to histograms.xml with this CL. There is data for Extensions.FunctionCalls, and, so far as I know, this code is alive. Since it uses the internal metrics url, not sure if it's okay to post a link here, but feel free to ping me for it.
histograms.xml and extension_function_histogram_value.h LGTM
The CQ bit was checked by vadimgo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/627573002/1030001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/75866) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vadimgo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/627573002/1040001
The CQ bit was checked by vadimgo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/627573002/1060001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running git apply --index -3 -p1;
error: patch failed:
extensions/browser/extension_function_histogram_value.h:969
Falling back to three-way merge...
Applied patch to 'extensions/browser/extension_function_histogram_value.h'
with conflicts.
U extensions/browser/extension_function_histogram_value.h
Patch: extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
8d0b10c98a5dc455e7b09e4952b17662b3737042..1af3d2dd3dc0a603c86af6a741b9a30a801c8a2c
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -969,6 +969,7 @@ enum HistogramValue {
INPUTMETHODPRIVATE_GETINPUTMETHODCONFIG,
WALLPAPERPRIVATE_GETSYNCSETTING,
COPRESENCE_SETAUTHTOKEN,
+ CAST_CHANNEL_SETAUTHORITYKEYS,
// Last entry: Add new entries above and ensure to update
// tools/metrics/histograms/histograms.xml.
ENUM_BOUNDARY
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by vadimgo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/627573002/1060001
Message was sent while issue was closed.
Committed patchset #26 (id:1060001)
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/df6b610eb8fb8cdaac5eb6b16aafefac3c90adb9 Cr-Commit-Position: refs/heads/master@{#302141} |
