Refactoring of Cast-related crypto code

Refactoring of Cast-related crypto code to use the same certificate validation logic in chrome.networkingPrivate API and Cast Channel authentication.

Here's what's being done here:
* Code from cast_auth_util_nss/openssl formed the basis a common Cast device validation component in /src/extensions/common/cast/cast_cert_validator*, and is now being extensively cleaned up in response to rsleevi's comments in this CL.
* Both networking_private_crypto* and cast_auth_util* have been updated to use the new common code.
* The current D-Bus-based implementation of VerifyDestination is going away per discussion with ChromeOS team, and is replaced with in-Chrome code in networking_private crypto*.

BUG=442650
Committed: https://crrev.com/ed1e90f4f980709cef6a8a9c7e0f64cfe5578cdd
Cr-Commit-Position: refs/heads/master@{#311460}

[sheretov, 2014-12-12 22:05:38]

sheretov@chromium.org changed reviewers:
+ mef@chromium.org, stevenjb@chromium.org

[sheretov, 2014-12-12 22:08:50]

Please take a look at this CL, implementing cast crypto refactoring we discussed
a few weeks ago.  ChromeOS unit tests are still broken - I'll need help from
stevenjb to get them working properly.

[stevenjb, 2014-12-15 17:35:02]

stevenjb@chromium.org changed reviewers:
+ rsleevi@chromium.org

[stevenjb, 2014-12-15 17:35:03]

The structural changes look mostly fine to me, but I'm not familiar enough with
the certificate specific code to review that.
+rsleevi for the cert code, especially wrt the code in src/extensions.

https://codereview.chromium.org/792353002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc
(right):

https://codereview.chromium.org/792353002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc:11:
class NetworkingPrivateCredentialsGetterCrOs
Class name should match filename (and we tend to spell out Chromeos, lengthy as
it is).

https://codereview.chromium.org/792353002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc:35:
callback.Run("", kErrorNotImplemented);
This won't break anything in the cast extension?

[sheretov, 2014-12-15 20:17:34]

sheretov@chromium.org changed reviewers:
+ vadimgo@chromium.org

[sheretov, 2014-12-15 20:42:29]

+vadimgo

https://codereview.chromium.org/792353002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc
(right):

https://codereview.chromium.org/792353002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc:11:
class NetworkingPrivateCredentialsGetterCrOs
On 2014/12/15 17:35:03, stevenjb wrote:
> Class name should match filename (and we tend to spell out Chromeos, lengthy
as
> it is).

Done.

https://codereview.chromium.org/792353002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc:35:
callback.Run("", kErrorNotImplemented);
On 2014/12/15 17:35:02, stevenjb wrote:
> This won't break anything in the cast extension?

Nope.

[Ryan Sleevi, 2014-12-15 21:15:35]

Add a bug # :)

[Ryan Sleevi, 2014-12-15 21:43:01]

You need to significantly expand upon your commit message.

It should have:

A brief (ideally < 70 chars) one liner

followed by a more fleshed out explanation of what the issue is and how your CL
sets about to fix this.

Followed by a link to a bug.

I highlighted several anti-patterns I found. I suspect I did not get every
instance. Please do a thorough self-review for the patterns noted below before
the next CL.

https://codereview.chromium.org/792353002/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc
(right):

https://codereview.chromium.org/792353002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc:35:
callback.Run("", kErrorNotImplemented);
s/""/std::string()/

https://codereview.chromium.org/792353002/diff/100001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/792353002/diff/100001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:351: ['(OS=="win" or OS=="mac" or chromeos==1) and
use_openssl==1', {
It's not entirely clear why the chromeos addition is correct here. Could you
explain?

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:36: "description": "An
array of PEM-encoded X.509 intermediate certificate authority certificates."
Documentation: PEM encoding can either mean with the PEM header/footer
("----BEGIN CERTIFICATE-----") or simply base-64 encoded. Because it's an array,
it's a bit ambiguous. It might help to documented this more explicitly (this is
sort of ambiguous in the other things too; for example, an RSA key can have the
form "PRIVATE KEY" or "RSA PRIVATE KEY" depending on the library you're using)

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
File
chrome/common/extensions/api/networking_private/networking_private_crypto.cc
(right):

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:45:
LOG(ERROR) << "Failed to parse certificate: ";
WARNING: Include braces around single line LOG statements as a general code
sanity. This is true for all macros - it helps to be explicit in the event the
macro is ever optimized out to a noop.

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:73:
.Success();
nit: Did clang-format do this? A modern one? I thought it was tweaked to prefer

verification_context->VerifySignatureOverData(
    signature, data).Success();

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.h (right):

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:17: struct VerificationResult {
Document

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:46: class CertVerificationContext {
Document

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:51: virtual VerificationResult
VerifySignatureOverData(
Document

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:54: virtual std::string
getCommonName() const = 0;
NAMING: This does not conform to the C++ style guide -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

s/getCommonName/GetCommonName()/

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:54: virtual std::string
getCommonName() const = 0;
Document

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:60: VerificationResult
VerifyCert(const base::StringPiece& device_cert,
Document

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:60: VerificationResult
VerifyCert(const base::StringPiece& device_cert,
naming: VerifyDeviceCert? VerifyDevice?

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:62: CertVerificationContext**
out_context);
API DESIGN: Don't return pointers to pointers, especially when the caller must
clear.

Instead, take a pointer to a scoped_ptr.

scoped_ptr<CastVerificationContext>* out_context

NAMING: s/out_context/context/ :)

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_nss.cc (right):

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:31: explicit
CertVerificationContextNSS(CERTCertificate* x509) : x509_(x509) {}
NAMING: s/x509/certificate/ (throughout)

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:67: return "";
s/""/std::string()

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:92: VLOG(1) << "Response has "
<< num_intermediates << " intermediates";
SPAM: This is spammy. Don't log like this. It doesn't tell you anything, and it
adds to bloat.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:149: VLOG(1) << "Cert signed
by trusted CA";
SPAM: This is also spammy. Considering you don't log the early returns (101-103,
124-126), this also doesn't tell meaningful information - it still forces an
inference.

But just get rid of it.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_openssl.cc (right):

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:39: LOG(ERROR) << "Failed
to extract device certificate public key.";
SPAM: Unnecessary, covered on line 41.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:49: << public_key_type <<
" instead.";
SPAM: Unnecessary, line 51 covers this.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:79: LOG(ERROR) <<
"Certificate does not have common name.";
SPAM

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:80: return "";
s/""/std::string()

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:85:
WriteInto(&common_name, common_name_length + 1),
DANGER: You're adding before integer promotion, thus may overflow into a
negative value if common_name_length == INT_MAX.

While this should crash do to the OOM handler, you're better off making your
promotion explicit

static_cast<size_t>(common_name_length) + 1

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:87:
DCHECK_EQ((int)common_name.size(), common_name_length);
STYLE: static_cast<int>(common_name.size())

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:89: LOG(ERROR) <<
"Certificate does not have common name.";
SPAM - and also false (considering line 82)

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:111: if (ica_certs.size()
<= 0) {
BUG: It's impossible for a size_t to ever be < 0

if (ica_certs.empty())

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:130: LOG(ERROR) << "Failed
to import trusted public key.";
SPAM

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:131: return
VerificationResult("failed to import trusted public key.",
GRAMMAR: s/failed/Failed/

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:137: LOG(ERROR) << "Failed
to initialize EVP_PKEY";
SPAM

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:139: "failed to initialize
EVP_PKEY.",
GRAMMAR: s/failed/Failed/

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:150: LOG(ERROR) << "Failed
to parse certificate.";
SPAM

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:151: return
VerificationResult("failed to parse  device certificate.",
GRAMMAR: s/failed/Failed/

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:151: return
VerificationResult("failed to parse  device certificate.",
GRAMMAR: s/parse  device/parse device/

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:157: LOG(ERROR) <<
"Certificate is not issued by a trusted CA.";
SPAM

[vadimgo, 2014-12-15 21:59:55]

vadimgo@chromium.org changed reviewers:
+ kmarshall@chromium.org, mfoltz@chromium.org

[sheretov, 2014-12-16 08:44:22]

Cleanup of cert_cert_validator in response to rsleevi's comments.  vadimgo, you
might want to take a closer look, as a lot of the original code that got cleaned
up came from cast_auth_util*.

https://codereview.chromium.org/792353002/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc
(right):

https://codereview.chromium.org/792353002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_chromeos.cc:35:
callback.Run("", kErrorNotImplemented);
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> s/""/std::string()/

Done.

https://codereview.chromium.org/792353002/diff/100001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/792353002/diff/100001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:351: ['(OS=="win" or OS=="mac" or chromeos==1) and
use_openssl==1', {
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> It's not entirely clear why the chromeos addition is correct here. Could you
> explain?

This CL moves ChromeOS to the same implementation of networkingPrivate crypto as
Windows and Mac.  Previously, ChromeOS had a different implementation that was
accessed through a D-Bus interface. The shared openssl (or nss) implementation
is contained in chrome_common_networking_private_sources_xxx.

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:36: "description": "An
array of PEM-encoded X.509 intermediate certificate authority certificates."
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> Documentation: PEM encoding can either mean with the PEM header/footer
> ("----BEGIN CERTIFICATE-----") or simply base-64 encoded. Because it's an
array,
> it's a bit ambiguous. It might help to documented this more explicitly (this
is
> sort of ambiguous in the other things too; for example, an RSA key can have
the
> form "PRIVATE KEY" or "RSA PRIVATE KEY" depending on the library you're using)

Done.

Fixed the publicKey field's description as well.  It is a Base64-encoded
RSAPublicKey - not PEM-encoded.

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
File
chrome/common/extensions/api/networking_private/networking_private_crypto.cc
(right):

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:45:
LOG(ERROR) << "Failed to parse certificate: ";
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> WARNING: Include braces around single line LOG statements as a general code
> sanity. This is true for all macros - it helps to be explicit in the event the
> macro is ever optimized out to a noop.

Done.

https://codereview.chromium.org/792353002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:73:
.Success();
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> nit: Did clang-format do this? A modern one? I thought it was tweaked to
prefer
> 
> verification_context->VerifySignatureOverData(
>     signature, data).Success();

Sure did.  I'm not a big fan myself, but I'm training myself to accept
clang-format's verdicts :)

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.h (right):

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:17: struct VerificationResult {
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> Document

Done.  And consolidated error types into more sensible buckets.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:46: class CertVerificationContext {
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:51: virtual VerificationResult
VerifySignatureOverData(
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:54: virtual std::string
getCommonName() const = 0;
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> NAMING: This does not conform to the C++ style guide -
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names
> 
> s/getCommonName/GetCommonName()/

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:54: virtual std::string
getCommonName() const = 0;
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:60: VerificationResult
VerifyCert(const base::StringPiece& device_cert,
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> Document

VerifyDeviceCert

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:60: VerificationResult
VerifyCert(const base::StringPiece& device_cert,
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:62: CertVerificationContext**
out_context);
On 2014/12/15 21:42:59, Ryan Sleevi wrote:
> API DESIGN: Don't return pointers to pointers, especially when the caller must
> clear.
> 
> Instead, take a pointer to a scoped_ptr.
> 
> scoped_ptr<CastVerificationContext>* out_context
> 
> NAMING: s/out_context/context/ :)

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_nss.cc (right):

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:31: explicit
CertVerificationContextNSS(CERTCertificate* x509) : x509_(x509) {}
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> NAMING: s/x509/certificate/ (throughout)

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:67: return "";
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> s/""/std::string()

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:92: VLOG(1) << "Response has "
<< num_intermediates << " intermediates";
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> SPAM: This is spammy. Don't log like this. It doesn't tell you anything, and
it
> adds to bloat.

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:149: VLOG(1) << "Cert signed
by trusted CA";
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> SPAM: This is also spammy. Considering you don't log the early returns
(101-103,
> 124-126), this also doesn't tell meaningful information - it still forces an
> inference.
> 
> But just get rid of it.

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_openssl.cc (right):

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:39: LOG(ERROR) << "Failed
to extract device certificate public key.";
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> SPAM: Unnecessary, covered on line 41.

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:49: << public_key_type <<
" instead.";
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> SPAM: Unnecessary, line 51 covers this.

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:79: LOG(ERROR) <<
"Certificate does not have common name.";
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> SPAM

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:80: return "";
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> s/""/std::string()

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:85:
WriteInto(&common_name, common_name_length + 1),
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> DANGER: You're adding before integer promotion, thus may overflow into a
> negative value if common_name_length == INT_MAX.
> 
> While this should crash do to the OOM handler, you're better off making your
> promotion explicit
> 
> static_cast<size_t>(common_name_length) + 1

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:87:
DCHECK_EQ((int)common_name.size(), common_name_length);
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> STYLE: static_cast<int>(common_name.size())

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:89: LOG(ERROR) <<
"Certificate does not have common name.";
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> SPAM - and also false (considering line 82)

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:111: if (ica_certs.size()
<= 0) {
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> BUG: It's impossible for a size_t to ever be < 0
> 
> if (ica_certs.empty())

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:130: LOG(ERROR) << "Failed
to import trusted public key.";
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> SPAM

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:131: return
VerificationResult("failed to import trusted public key.",
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> GRAMMAR: s/failed/Failed/

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:137: LOG(ERROR) << "Failed
to initialize EVP_PKEY";
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> SPAM

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:139: "failed to initialize
EVP_PKEY.",
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> GRAMMAR: s/failed/Failed/

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:150: LOG(ERROR) << "Failed
to parse certificate.";
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> SPAM

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:151: return
VerificationResult("failed to parse  device certificate.",
On 2014/12/15 21:43:00, Ryan Sleevi wrote:
> GRAMMAR: s/failed/Failed/

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:151: return
VerificationResult("failed to parse  device certificate.",
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> GRAMMAR: s/parse  device/parse device/

Done.

https://codereview.chromium.org/792353002/diff/100001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:157: LOG(ERROR) <<
"Certificate is not issued by a trusted CA.";
On 2014/12/15 21:43:01, Ryan Sleevi wrote:
> SPAM

Done.

[sheretov, 2014-12-16 08:46:49]

sheretov@chromium.org changed reviewers:
+ jochen@chromium.org, rockot@chromium.org

[mark a. foltz, 2014-12-18 00:43:42]

I don't follow the relationship of the new code in this patch to the existing
code in cast_channel.  Is this a rewrite?  It's risky to rewrite and refactor at
the same time and this particular patch adds some kluges like mapping error
enums to fit differing APIs.

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:60: AuthResult
TranslateVerificationError(cast_crypto::VerificationResult error) {
Better named TranslateVerificationResult since it's called with all results (not
just errors).

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:64: static
AuthResult::ErrorType error_type_map[] = {
We generally use switch statements for this type of mapping.  This seems less
readable (since I don't know what I'm mapping from).

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:73: ?
AuthResult::ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT
How will AuthResult::ErrorType and error.error_type be kept up to date?  Can you
add a comment in the appropriate place in cast_crypto::VerificationResult to
keep AuthResult::ErrorType in sync?

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.cc:12: : error_type(ERROR_NONE),
library_error_code(0) {
Consider using a delegated constructor here to reduce code duplication.

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_nss.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:5: #include
"extensions/common/cast/cast_cert_validator.h"
Is this new code or is there a diff of code that was checked into cast_channel/?

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_openssl.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:5: #include
"extensions/common/cast/cast_cert_validator.h"
Similar question here.

[sheretov, 2015-01-03 03:18:02]

Updated the CL with changes in response to Mark's comments.

A bit of info on the refactoring vs. rewrite question: the CL started mostly as
a refactor with crypto code from cast_channel implementation forming the core of
the new common implementation.
Ryan had a number of comments on the original crypto code once it showed up as
new code in extensions/common/cast. With those comments addressed, the CL took
on more of a rewrite quality.

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:60: AuthResult
TranslateVerificationError(cast_crypto::VerificationResult error) {
On 2014/12/18 00:43:41, mark a. foltz wrote:
> Better named TranslateVerificationResult since it's called with all results
(not
> just errors).

Done.

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:64: static
AuthResult::ErrorType error_type_map[] = {
On 2014/12/18 00:43:41, mark a. foltz wrote:
> We generally use switch statements for this type of mapping.  This seems less
> readable (since I don't know what I'm mapping from).

Done.

https://codereview.chromium.org/792353002/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:73: ?
AuthResult::ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT
On 2014/12/18 00:43:41, mark a. foltz wrote:
> How will AuthResult::ErrorType and error.error_type be kept up to date?  Can
you
> add a comment in the appropriate place in cast_crypto::VerificationResult to
> keep AuthResult::ErrorType in sync?

Done in cast_cert_validator.h, but I think the compiler is more likely to help
catch breaking changes than the comment. Converting from an index-based mapping
to explicit switch above will help.

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.cc:12: : error_type(ERROR_NONE),
library_error_code(0) {
On 2014/12/18 00:43:42, mark a. foltz wrote:
> Consider using a delegated constructor here to reduce code duplication.

It looks about as verbose, but with an unsightly temporary empty string object:
     : VerificationResult("", ERROR_NONE, 0) {
... and then If I want to understand what the parameters map to, I have to look
for the other constructor.

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_nss.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:5: #include
"extensions/common/cast/cast_cert_validator.h"
On 2014/12/18 00:43:42, mark a. foltz wrote:
> Is this new code or is there a diff of code that was checked into
cast_channel/?

This code started out as a transplant from cast_channel implementation
(cast_auth_util_nss.cc), with obvious changes to work with a slightly different
API defined in cast_cert_validator.h.  Then rsleevi@ had a number of comments
about the code quality, which were resolved in this CL. Now it's *mostly* based
on the original code from cast_auth_util_nss.cc.

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_openssl.cc (right):

https://codereview.chromium.org/792353002/diff/140001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:5: #include
"extensions/common/cast/cast_cert_validator.h"
On 2014/12/18 00:43:42, mark a. foltz wrote:
> Similar question here.

Similar answer as with the NSS implementation.

[mef, 2015-01-05 17:24:09]

https://codereview.chromium.org/792353002/diff/220001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_verify_delegate_factory_impl.cc
(right):

https://codereview.chromium.org/792353002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_verify_delegate_factory_impl.cc:21:
#if defined(OS_CHROMEOS) || defined(OS_WIN) || defined(OSMACOSX)
I think stevenjb@ was doing some refactoring of this code, so this may now also
apply to LINUX.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.h (right):

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:88: VerificationResult
VerifyDeviceCert(
Is there a reason why VerifyDeviceCert shouldn't be a method of
CertVerificationContext?

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_nss.cc (right):

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:96: return VerificationResult(
nit: I think multiline if statements require curly braces according to coding
style. Also below.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:137:
scoped_ptr<CertVerificationContext> tmpPtr(
nit: no camel case, more descriptive name.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_openssl.cc (right):

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:104: if
(ica_public_key_der.empty())
nit: multiline if needs {}

[sheretov, 2015-01-05 22:43:03]

Styling fixes

https://codereview.chromium.org/792353002/diff/220001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_verify_delegate_factory_impl.cc
(right):

https://codereview.chromium.org/792353002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_verify_delegate_factory_impl.cc:21:
#if defined(OS_CHROMEOS) || defined(OS_WIN) || defined(OSMACOSX)
On 2015/01/05 17:24:08, mef wrote:
> I think stevenjb@ was doing some refactoring of this code, so this may now
also
> apply to LINUX.

Checked with stevenjb@.  Linux-related changes will go into a separate CL.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.h (right):

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:88: VerificationResult
VerifyDeviceCert(
On 2015/01/05 17:24:08, mef wrote:
> Is there a reason why VerifyDeviceCert shouldn't be a method of
> CertVerificationContext?

I experimented with that approach and ended up with more verbose code without a
clear benefit.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_nss.cc (right):

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:96: return VerificationResult(
On 2015/01/05 17:24:08, mef wrote:
> nit: I think multiline if statements require curly braces according to coding
> style. Also below.

Done.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_nss.cc:137:
scoped_ptr<CertVerificationContext> tmpPtr(
On 2015/01/05 17:24:08, mef wrote:
> nit: no camel case, more descriptive name.

Done.

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator_openssl.cc (right):

https://codereview.chromium.org/792353002/diff/220001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator_openssl.cc:104: if
(ica_public_key_der.empty())
On 2015/01/05 17:24:08, mef wrote:
> nit: multiline if needs {}

Done.

[stevenjb, 2015-01-05 22:53:21]

c/b/e/api/networking_private lgtm

[Ken Rockot(use gerrit already), 2015-01-05 23:16:07]

https://codereview.chromium.org/792353002/diff/280001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/792353002/diff/280001/extensions/extensions.g...
extensions/extensions.gyp:313: 'browser/api/audio/audio_service.cc',
It looks like you're introducing a dependency on extensions/browser from within
extensions/common. That's no good, but it seems like it's probably just an
accidental move that came along with the openssl condition?

[sheretov, 2015-01-06 00:33:14]

sheretov@chromium.org changed reviewers:
+ cpu@chromium.org
- jochen@chromium.org

[sheretov, 2015-01-06 00:33:30]

Fixed a rebase/merge error in extensions.gyp

https://codereview.chromium.org/792353002/diff/280001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/792353002/diff/280001/extensions/extensions.g...
extensions/extensions.gyp:313: 'browser/api/audio/audio_service.cc',
On 2015/01/05 23:16:07, Ken Rockot wrote:
> It looks like you're introducing a dependency on extensions/browser from
within
> extensions/common. That's no good, but it seems like it's probably just an
> accidental move that came along with the openssl condition?

Rebase/merge error.  Fixed.

[Ken Rockot(use gerrit already), 2015-01-06 04:06:56]

thanks. general extensions lgtm

[mef, 2015-01-06 20:57:34]

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.cc (right):

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.cc:17: : error_type(in_error_type),
error_message(in_error_message) {
missing error_code initializer.

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.h (right):

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:56: int library_error_code;
DISALLOW_COPY_AND_ASSIGN?

[sheretov, 2015-01-06 22:26:55]

Switched VerificationResult to use delegated constructors.

[sheretov, 2015-01-06 22:29:31]

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.cc (right):

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.cc:17: : error_type(in_error_type),
error_message(in_error_message) {
On 2015/01/06 20:57:34, mef wrote:
> missing error_code initializer.

Done.

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.h (right):

https://codereview.chromium.org/792353002/diff/290001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.h:56: int library_error_code;
On 2015/01/06 20:57:34, mef wrote:
> DISALLOW_COPY_AND_ASSIGN?

We return this by value.

[cpu_(ooo_6.6-7.5), 2015-01-08 18:11:17]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[sheretov, 2015-01-08 23:16:14]

The CQ bit was checked by sheretov@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:17:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792353002/310001

[commit-bot: I haz the power, 2015-01-08 23:27:46]

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

[commit-bot: I haz the power, 2015-01-08 23:27:48]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sheretov, 2015-01-09 00:15:27]

sheretov@chromium.org changed reviewers:
+ brettw@chromium.org

[sheretov, 2015-01-09 00:17:25]

sheretov@chromium.org changed required reviewers:
+ brettw@chromium.org, mfoltz@chromium.org, stevenjb@chromium.org

[sheretov, 2015-01-09 00:17:26]

brettw, please review changes to chrome/common/BUILD.gn

[sheretov, 2015-01-09 00:18:14]

sheretov@chromium.org changed required reviewers:
+ kmarshall@chromium.org

[sheretov, 2015-01-09 00:18:44]

sheretov@chromium.org changed required reviewers:
+ rockot@chromium.org
- kmarshall@chromium.org

[brettw, 2015-01-09 06:41:49]

lgtm

[mark a. foltz, 2015-01-13 21:27:46]

lgtm

https://codereview.chromium.org/792353002/diff/310001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/792353002/diff/310001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:153:
std::vector<std::string>(response.intermediate_certificate().begin(),
Is it necessary to duplicate all intermediate_certificate entries from
|response| here?  I suppose that these are fairly small strings (a few KB) so
don't feel strongly about optimizing this.

[sheretov, 2015-01-14 10:16:30]

The CQ bit was checked by sheretov@chromium.org

[commit-bot: I haz the power, 2015-01-14 10:17:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792353002/310001

[commit-bot: I haz the power, 2015-01-14 10:20:37]

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

[commit-bot: I haz the power, 2015-01-14 10:20:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[sheretov, 2015-01-14 11:07:54]

The CQ bit was checked by sheretov@chromium.org

[commit-bot: I haz the power, 2015-01-14 11:08:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792353002/350001

[commit-bot: I haz the power, 2015-01-14 12:10:07]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2015-01-14 12:11:23]

Patchset 19 (id:??) landed as
https://crrev.com/ed1e90f4f980709cef6a8a9c7e0f64cfe5578cdd
Cr-Commit-Position: refs/heads/master@{#311460}

[jochen (gone - plz use gerrit), 2015-01-14 12:25:47]

A revert of this CL (patchset #19 id:350001) has been created in
https://codereview.chromium.org/853663003/ by jochen@chromium.org.

The reason for reverting is: fails to compile on Linux ChromiumOS GN
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....