Implement crypto signature verification routines using OpenSSL.

Implement crypto signature verification routines using OpenSSL.

BUG=426948
R=mfoltz@chromium.org,wez@chromium.org,davidben@chromium.org

Committed: https://crrev.com/311d5b8414d195abf8346b48900661503b83be57
Cr-Commit-Position: refs/heads/master@{#302520}

[Kevin Marshall, 2014-10-29 00:04:45]

marshallk@google.com changed reviewers:
+ marshallk@google.com

[Kevin Marshall, 2014-10-29 00:04:46]

Unit tests not done yet - OpenSSL unit tests are broken in head and lkgr. Will
add them ASAP.

../../chrome/common/net/x509_certificate_model_unittest.cc:321:36: error: no
viable conversion from 'value_type' (aka 'scoped_refptr<net::X509Certificate>')
to 'const net::X509Certificate *'

[davidben, 2014-10-30 18:37:24]

Looks good. I have some minor comments, but I think this is just missing the
unit tests.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:28:
ERROR_NSS_CERT_PARSING_FAILED,
Nit: Maybe remove the NSS_ prefix from these since they're not NSS-specific
anymore.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_nss.cc:89:
SECKEY_ImportDERPublicKey(&trusted_ca_key_der, CKK_RSA));
Minor: While you're here, may as well check for ca_public_key being NULL on this
side too. (Fortunately, CERT_VerifySignedDataWithPublicKey does check for NULL
in the argument, so nothing bad should happen. And those keys are hard-coded
anyway.)

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:29: // * Verifies
that |auth_message.response.client_auth_certificate| is signed
Nit: s/auth_message.//

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:30: //   by the
trusted CA public key passed as |ca_public_key_bytes|.
Nit: ca_public_key_bytes is no longer a parameter.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:35: const
std::string& peer_cert) {
This should include crypto/openssl_util.h and add a

   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);

line at the top. This'll ensure that the OpenSSL error queue gets cleared on
function exit. And it'll print the errors on debug builds or when logging is
enabled, which is probably convenient.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:113: return
AuthResult::Create(
You could probably just chain these few together with ||s and let
OpenSSLErrStackTracer provide the context needed, but this is fine too.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:131: int result =
EVP_DigestVerifyFinal(
Actually, as of https://codereview.chromium.org/664663004/ this can just be

  if (!EVP_DigestVerifyFinal(

There was only one case left where it could return -1 and that's gone. (Going
forward, BoringSSL functions are going to try to avoid being tri-state like
that, but it'll take a while to go through all the legacy functions.)

[mark a. foltz, 2014-10-31 06:49:12]

There appears to be a duplicated sentence in the description :)

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:28:
ERROR_NSS_CERT_PARSING_FAILED,
On 2014/10/30 18:37:22, David Benjamin wrote:
> Nit: Maybe remove the NSS_ prefix from these since they're not NSS-specific
> anymore.

+1

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:45: return
AuthResult::Create(
The AuthResult API has evolved with vadimgo's patch, you may need to
reconstitute this method.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:54: const uint8_t*
ca_public_key_end = ca_ptr + ca_public_key_bytes.size();
Why are you calling size() here and length() below?

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:54: const uint8_t*
ca_public_key_end = ca_ptr + ca_public_key_bytes.size();
What is ca_public_key_end pointing to?  It seems to be pointing just past the
end of ca_public_key_bytes_data - but there can be no valid key contents there?

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:64: if
(!ca_public_key ||
What type is ca_public_key - I assume it has a !operator?

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:75: const uint8_t*
client_cert_end =
Similar questions about the use of client_cert_end

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:89: if
(X509_verify(client_cert.get(), ca_public_key.get()) <= 0) {
Does X509_verify return specific error codes?  If so they should be passed with
the AuthResult.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:105:
AuthResult::ERROR_NSS_CANNOT_EXTRACT_PUBLIC_KEY);
Is there an OpenSSL equivalent to PORT_GetError() that would allow us to pass a
specific failure reason back through AuthResult for all of these library calls? 
We want to make sure to include them in our channel event logs and OnError
events.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:143: AuthResult
AuthenticateChallengeReply(const CastMessage& challenge_reply,
This appears to be identical to the same method in _nss.cc.  Hoist it out into a
utility class and eliminate the duplicated code?

[mark a. foltz, 2014-10-31 06:52:22]

One more comment - we should add an openssl_error_code property to LastErrors,
AuthResult and ErrorInfo to propagate errors into the channel logs and up to the
extension.  As long the errors are captured in AuthResult, then the other
changes can be done in a separate patch.

[Kevin M, 2014-10-31 21:39:38]

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:28:
ERROR_NSS_CERT_PARSING_FAILED,
On 2014/10/31 06:49:11, mark a. foltz wrote:
> On 2014/10/30 18:37:22, David Benjamin wrote:
> > Nit: Maybe remove the NSS_ prefix from these since they're not NSS-specific
> > anymore.
> 
> +1

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:28:
ERROR_NSS_CERT_PARSING_FAILED,
On 2014/10/30 18:37:22, David Benjamin wrote:
> Nit: Maybe remove the NSS_ prefix from these since they're not NSS-specific
> anymore.

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_nss.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_nss.cc:89:
SECKEY_ImportDERPublicKey(&trusted_ca_key_der, CKK_RSA));
On 2014/10/30 18:37:22, David Benjamin wrote:
> Minor: While you're here, may as well check for ca_public_key being NULL on
this
> side too. (Fortunately, CERT_VerifySignedDataWithPublicKey does check for NULL
> in the argument, so nothing bad should happen. And those keys are hard-coded
> anyway.)

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:29: // * Verifies
that |auth_message.response.client_auth_certificate| is signed
On 2014/10/30 18:37:23, David Benjamin wrote:
> Nit: s/auth_message.//

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:30: //   by the
trusted CA public key passed as |ca_public_key_bytes|.
On 2014/10/30 18:37:23, David Benjamin wrote:
> Nit: ca_public_key_bytes is no longer a parameter.

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:35: const
std::string& peer_cert) {
On 2014/10/30 18:37:23, David Benjamin wrote:
> This should include crypto/openssl_util.h and add a
> 
>    crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
> 
> line at the top. This'll ensure that the OpenSSL error queue gets cleared on
> function exit. And it'll print the errors on debug builds or when logging is
> enabled, which is probably convenient.

That's neat, Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:45: return
AuthResult::Create(
On 2014/10/31 06:49:11, mark a. foltz wrote:
> The AuthResult API has evolved with vadimgo's patch, you may need to
> reconstitute this method.

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:54: const uint8_t*
ca_public_key_end = ca_ptr + ca_public_key_bytes.size();
On 2014/10/31 06:49:11, mark a. foltz wrote:
> Why are you calling size() here and length() below?

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:54: const uint8_t*
ca_public_key_end = ca_ptr + ca_public_key_bytes.size();
On 2014/10/31 06:49:11, mark a. foltz wrote:
> What is ca_public_key_end pointing to?  It seems to be pointing just past the
> end of ca_public_key_bytes_data - but there can be no valid key contents
there?

It's the expected value of cert_ptr after d2i_X509, which increments the cert
data ptr on successful read.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:54: const uint8_t*
ca_public_key_end = ca_ptr + ca_public_key_bytes.size();
On 2014/10/31 06:49:11, mark a. foltz wrote:
> Why are you calling size() here and length() below?

Good catch, done

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:64: if
(!ca_public_key ||
On 2014/10/31 06:49:11, mark a. foltz wrote:
> What type is ca_public_key - I assume it has a !operator?

It's a scoped_ptr. I can't find an operator! defined for scoped_ptr, but I've
seen it being used this way in other code that uses OpenSSL.

https://code.google.com/p/chromium/codesearch#search/&q=scopedevp_pkey%20%22!...

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:75: const uint8_t*
client_cert_end =
On 2014/10/31 06:49:11, mark a. foltz wrote:
> Similar questions about the use of client_cert_end

Acknowledged.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:89: if
(X509_verify(client_cert.get(), ca_public_key.get()) <= 0) {
On 2014/10/31 06:49:11, mark a. foltz wrote:
> Does X509_verify return specific error codes?  If so they should be passed
with
> the AuthResult.

From what I could tell, it's one or zero "except in certain exceptional
circumstances" (according to the OpenSSL wiki).

But, from David's comment in this CL: "Going forward, BoringSSL functions are
going to try to avoid being tri-state like that, but it'll take a while to go
through all the legacy functions."

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:113: return
AuthResult::Create(
On 2014/10/30 18:37:23, David Benjamin wrote:
> You could probably just chain these few together with ||s and let
> OpenSSLErrStackTracer provide the context needed, but this is fine too.

Having conditional blocks for each step of the way allows us to add more
detailed logging info to AuthResult, which is useful for us.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:131: int result =
EVP_DigestVerifyFinal(
On 2014/10/30 18:37:22, David Benjamin wrote:
> Actually, as of https://codereview.chromium.org/664663004/ this can just be
> 
>   if (!EVP_DigestVerifyFinal(
> 
> There was only one case left where it could return -1 and that's gone. (Going
> forward, BoringSSL functions are going to try to avoid being tri-state like
> that, but it'll take a while to go through all the legacy functions.)

Done.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:143: AuthResult
AuthenticateChallengeReply(const CastMessage& challenge_reply,
On 2014/10/31 06:49:11, mark a. foltz wrote:
> This appears to be identical to the same method in _nss.cc.  Hoist it out into
a
> utility class and eliminate the duplicated code?

Done.

[Kevin M, 2014-10-31 21:57:24]

Added unit tests.

[mark a. foltz, 2014-10-31 22:29:26]

A few comments about handling errors from the openssl library and unit tests. 
If you want to submit this and iterate on error handling in a later patch that
would be okay with me.  Let me know.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:60: // of OpenSSL errors.
So each entry is just part of a stack trace for a single error?  Are the entries
of the form:

file.cc:XXX

Is there an error code or other explanation of the error condition?

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:61:
std::vector<std::pair<std::string, int>> openssl_errors;
Slight preference for declaring a struct to hold these entries, so that code
accessing them is more readable.  E.g.,

struct OpenSslStackElement {
  std::string filename;
  int line_number;
};

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:67: const
std::vector<std::pair<std::string, int>>& openssl_errors);
Maybe this should be openssl_error_stack?

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:34: while
(ERR_get_error_line(&filename, &line_num) != 0)  {
Do we want to capture the return value from this to get the packed_error integer
from OpenSSL?

Even better, there are routines in err.c to turn them into human readable
strings, which would probably help us understand what happened.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc
(right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:311:
class CastSocketOpenSSLTest : public testing::Test {
CastAuthUtilOpenSslTest?

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:339:
static void MangleString(std::string* str) {
I like this test design.  Will it only be executed in openssl=1 builds?  Is
there a way we can have it run for NSS builds as well?

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:384:
TEST_F(CastSocketOpenSSLTest, VerifyBadPeerCert) {
I counted 10 error conditions in the implementation, but there are only 4 test
cases for errors here.  Are there any other error conditions that can be
simulated to exercise additional code paths?  If the error is internal to the
openssl implementation and not dependent on input I would understand (I assume
there is no simple way to inject errors into it.)

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:180: if (result.success()) {
Did you intend to submit this change?
If so, is error_message set on success?

[Kevin M, 2014-11-01 00:03:05]

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:60: // of OpenSSL errors.
On 2014/10/31 22:29:25, mark a. foltz wrote:
> So each entry is just part of a stack trace for a single error?  Are the
entries
> of the form:
> 
> file.cc:XXX
> 
> Is there an error code or other explanation of the error condition?

It's a stack of errors.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

"The typical behaviour is that an error will occur deep in a call queue and that
code will push an error onto the error queue. As the error queue unwinds, other
functions will push their own errors. Thus, the "least recent" error is the most
specific and the other errors will provide a backtrace of sorts."

There is an error code, but davidben advised against sending them over the wire.
He recommended we send filename/line pairs instead.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:61:
std::vector<std::pair<std::string, int>> openssl_errors;
On 2014/10/31 22:29:25, mark a. foltz wrote:
> Slight preference for declaring a struct to hold these entries, so that code
> accessing them is more readable.  E.g.,
> 
> struct OpenSslStackElement {
>   std::string filename;
>   int line_number;
> };

How's this?

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:67: const
std::vector<std::pair<std::string, int>>& openssl_errors);
On 2014/10/31 22:29:25, mark a. foltz wrote:
> Maybe this should be openssl_error_stack?

Done.

[davidben, 2014-11-01 00:19:33]

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

[davidben, 2014-11-01 00:19:35]

I haven't had a chance to look at the new version carefully, but responding to a
few comments before I leave for the day since I noticed them:

Also, should we perhaps split out all the error-reporting complexity into a
separate CL and just land the verification + test, so that there's some hope of
merging it to 39? Or are we completely giving up on that and okay with shipping
a regression in 39 on Mac where it doesn't do what I assume is a
security-relevant verification? (+rsleevi)

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:89: if
(X509_verify(client_cert.get(), ca_public_key.get()) <= 0) {
On 2014/10/31 21:39:37, Kevin M wrote:
> On 2014/10/31 06:49:11, mark a. foltz wrote:
> > Does X509_verify return specific error codes?  If so they should be passed
> with
> > the AuthResult.
> 
> From what I could tell, it's one or zero "except in certain exceptional
> circumstances" (according to the OpenSSL wiki).
> 
> But, from David's comment in this CL: "Going forward, BoringSSL functions are
> going to try to avoid being tri-state like that, but it'll take a while to go
> through all the legacy functions."

For stuff in crypto/, whether it's 0 or -1 is almost always uninteresting and is
going to be redundant with the error queue.

(As it turns out, it looks like X509_verify in BoringSSL already isn't tri-state
anymore.)

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:131: int result =
EVP_DigestVerifyFinal(
On 2014/10/31 21:39:38, Kevin M wrote:
> On 2014/10/30 18:37:22, David Benjamin wrote:
> > Actually, as of https://codereview.chromium.org/664663004/ this can just be
> > 
> >   if (!EVP_DigestVerifyFinal(
> > 
> > There was only one case left where it could return -1 and that's gone.
(Going
> > forward, BoringSSL functions are going to try to avoid being tri-state like
> > that, but it'll take a while to go through all the legacy functions.)
> 
> Done.

Oops. It occurred to me that we probably want to leave merging this as an
option. Mind switching that back? Apologies for the churn.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:60: // of OpenSSL errors.
On 2014/11/01 00:03:05, Kevin M wrote:
> On 2014/10/31 22:29:25, mark a. foltz wrote:
> > So each entry is just part of a stack trace for a single error?  Are the
> entries
> > of the form:
> > 
> > file.cc:XXX
> > 
> > Is there an error code or other explanation of the error condition?
> 
> It's a stack of errors.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> 
> "The typical behaviour is that an error will occur deep in a call queue and
that
> code will push an error onto the error queue. As the error queue unwinds,
other
> functions will push their own errors. Thus, the "least recent" error is the
most
> specific and the other errors will provide a backtrace of sorts."
> 
> There is an error code, but davidben advised against sending them over the
wire.
> He recommended we send filename/line pairs instead.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

Including the error values is fine as long as it's clearly documented they
should not be used for anything but manual debugging. (Neither should
file/line.) The values are not stable and will change from Chrome release to
release. If you're not including the version number in these, they wouldn't be
very useful.

Really file + line is also not stable either, so the same caveat applies to that
too.

(Honestly, just including the top one is probably fine. X509_verify is going to
push a silly ERR_R_EVP_LIB to mask the actual crypto failure, but I can probably
just go and remove that if agl agrees. I've never understood the point...)

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:34: while
(ERR_get_error_line(&filename, &line_num) != 0)  {
On 2014/10/31 22:29:25, mark a. foltz wrote:
> Do we want to capture the return value from this to get the packed_error
integer
> from OpenSSL?
> 
> Even better, there are routines in err.c to turn them into human readable
> strings, which would probably help us understand what happened.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

Please do not use ERR_error_string and use ERR_error_string_n instead.
ERR_error_string is not thread-safe. (On that note, I should excise its use from
net...)

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:135: if (!ctx) {
135 through 150 can be coalesced into one block now that you're capturing all
the errors anyway.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc
(right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:339:
static void MangleString(std::string* str) {
On 2014/10/31 22:29:26, mark a. foltz wrote:
> I like this test design.  Will it only be executed in openssl=1 builds?  Is
> there a way we can have it run for NSS builds as well?

+1. These tests don't look OpenSSL-specific.

https://codereview.chromium.org/687733004/diff/100001/extensions/common/api/c...
File extensions/common/api/cast_channel/logging.proto (right):

https://codereview.chromium.org/687733004/diff/100001/extensions/common/api/c...
extensions/common/api/cast_channel/logging.proto:112: optional int32 line = 2;
Does this send the Chrome version? These aren't going to be meaningful without
it either.

https://codereview.chromium.org/687733004/diff/100001/extensions/common/api/c...
extensions/common/api/cast_channel/logging.proto:135: repeated OpenSSLError
openssl_error = 13;
This should have a comment that this is specifically for manual debugging and no
code should ever programmatically do anything with these values. Constraining
BoringSSL to file names and line numbers of errors would be even worse than
constraining the reason codes.

(To be clear, neither of them are guaranteed to be stable across versions, so
that comment would be true for both.)

[mark a. foltz, 2014-11-01 19:59:40]

I would be fine deferring more refined error handling to another patch, to
improve the chance this has for getting merged.

[Kevin M, 2014-11-03 18:31:46]

All right, I made the suggested changes, and moved the error reporting to a
separate Git branch. I also made the new unit tests security lib agnostic and
exercised them against NSS and OpenSSL to ensure consistency. PTAL.

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/40001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:131: int result =
EVP_DigestVerifyFinal(
On 2014/11/01 00:19:34, David Benjamin wrote:
> On 2014/10/31 21:39:38, Kevin M wrote:
> > On 2014/10/30 18:37:22, David Benjamin wrote:
> > > Actually, as of https://codereview.chromium.org/664663004/ this can just
be
> > > 
> > >   if (!EVP_DigestVerifyFinal(
> > > 
> > > There was only one case left where it could return -1 and that's gone.
> (Going
> > > forward, BoringSSL functions are going to try to avoid being tri-state
like
> > > that, but it'll take a while to go through all the legacy functions.)
> > 
> > Done.
> 
> Oops. It occurred to me that we probably want to leave merging this as an
> option. Mind switching that back? Apologies for the churn.

Done.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:60: // of OpenSSL errors.
On 2014/11/01 00:19:34, David Benjamin wrote:
> On 2014/11/01 00:03:05, Kevin M wrote:
> > On 2014/10/31 22:29:25, mark a. foltz wrote:
> > > So each entry is just part of a stack trace for a single error?  Are the
> > entries
> > > of the form:
> > > 
> > > file.cc:XXX
> > > 
> > > Is there an error code or other explanation of the error condition?
> > 
> > It's a stack of errors.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> > 
> > "The typical behaviour is that an error will occur deep in a call queue and
> that
> > code will push an error onto the error queue. As the error queue unwinds,
> other
> > functions will push their own errors. Thus, the "least recent" error is the
> most
> > specific and the other errors will provide a backtrace of sorts."
> > 
> > There is an error code, but davidben advised against sending them over the
> wire.
> > He recommended we send filename/line pairs instead.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> 
> Including the error values is fine as long as it's clearly documented they
> should not be used for anything but manual debugging. (Neither should
> file/line.) The values are not stable and will change from Chrome release to
> release. If you're not including the version number in these, they wouldn't be
> very useful.
> 
> Really file + line is also not stable either, so the same caveat applies to
that
> too.
> 
> (Honestly, just including the top one is probably fine. X509_verify is going
to
> push a silly ERR_R_EVP_LIB to mask the actual crypto failure, but I can
probably
> just go and remove that if agl agrees. I've never understood the point...)

Done in a separate Git branch.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc
(right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:339:
static void MangleString(std::string* str) {
On 2014/11/01 00:19:35, David Benjamin wrote:
> On 2014/10/31 22:29:26, mark a. foltz wrote:
> > I like this test design.  Will it only be executed in openssl=1 builds?  Is
> > there a way we can have it run for NSS builds as well?
> 
> +1. These tests don't look OpenSSL-specific.

Done.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:339:
static void MangleString(std::string* str) {
On 2014/10/31 22:29:26, mark a. foltz wrote:
> I like this test design.  Will it only be executed in openssl=1 builds?  Is
> there a way we can have it run for NSS builds as well?

Done.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl_unittest.cc:384:
TEST_F(CastSocketOpenSSLTest, VerifyBadPeerCert) {
On 2014/10/31 22:29:25, mark a. foltz wrote:
> I counted 10 error conditions in the implementation, but there are only 4 test
> cases for errors here.  Are there any other error conditions that can be
> simulated to exercise additional code paths?  If the error is internal to the
> openssl implementation and not dependent on input I would understand (I assume
> there is no simple way to inject errors into it.)

The other errors that aren't triggered are internal to OSSL.

I'm making this test NSS/OpenSSL agnostic so it seems like a grey box approach
would be fine?

https://codereview.chromium.org/687733004/diff/100001/extensions/common/api/c...
File extensions/common/api/cast_channel/logging.proto (right):

https://codereview.chromium.org/687733004/diff/100001/extensions/common/api/c...
extensions/common/api/cast_channel/logging.proto:112: optional int32 line = 2;
On 2014/11/01 00:19:35, David Benjamin wrote:
> Does this send the Chrome version? These aren't going to be meaningful without
> it either.

The Chrome version is included in the feedback report metadata.

FYI, the error-reporting changes have been moved to a separate Git branch, which
I'll put out for review once I land this CL.

https://codereview.chromium.org/687733004/diff/100001/extensions/common/api/c...
extensions/common/api/cast_channel/logging.proto:135: repeated OpenSSLError
openssl_error = 13;
On 2014/11/01 00:19:35, David Benjamin wrote:
> This should have a comment that this is specifically for manual debugging and
no
> code should ever programmatically do anything with these values. Constraining
> BoringSSL to file names and line numbers of errors would be even worse than
> constraining the reason codes.
> 
> (To be clear, neither of them are guaranteed to be stable across versions, so
> that comment would be true for both.)

Done.

[Kevin M, 2014-11-03 18:32:12]

kmarshall@chromium.org changed reviewers:
+ rockot@chromium.org

[Kevin M, 2014-11-03 18:32:12]

+rockot for gyp review (thanks!)

[Kevin M, 2014-11-03 18:35:58]

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:34: while
(ERR_get_error_line(&filename, &line_num) != 0)  {
On 2014/11/01 00:19:34, David Benjamin wrote:
> On 2014/10/31 22:29:25, mark a. foltz wrote:
> > Do we want to capture the return value from this to get the packed_error
> integer
> > from OpenSSL?
> > 
> > Even better, there are routines in err.c to turn them into human readable
> > strings, which would probably help us understand what happened.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> 
> Please do not use ERR_error_string and use ERR_error_string_n instead.
> ERR_error_string is not thread-safe. (On that note, I should excise its use
from
> net...)

? The code is calling ERR_get_error_line(), which does not have a deprecation
notice in err.h? Does it also have thread safety issues?

[Ken Rockot(use gerrit already), 2014-11-03 18:59:04]

On 2014/11/03 18:35:58, Kevin M wrote:
>
https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
> File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):
> 
>
https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
> extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:34: while
> (ERR_get_error_line(&filename, &line_num) != 0)  {
> On 2014/11/01 00:19:34, David Benjamin wrote:
> > On 2014/10/31 22:29:25, mark a. foltz wrote:
> > > Do we want to capture the return value from this to get the packed_error
> > integer
> > > from OpenSSL?
> > > 
> > > Even better, there are routines in err.c to turn them into human readable
> > > strings, which would probably help us understand what happened.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> > 
> > Please do not use ERR_error_string and use ERR_error_string_n instead.
> > ERR_error_string is not thread-safe. (On that note, I should excise its use
> from
> > net...)
> 
> ? The code is calling ERR_get_error_line(), which does not have a deprecation
> notice in err.h? Does it also have thread safety issues?

lgtm but I think you also want to update BUILD.gn?

[davidben, 2014-11-03 19:01:50]

Thanks! lgtm with the BUILD.gn update.

(Might want to do another run of try jobs, by the way. They seem to have fallen
over.)

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:34: while
(ERR_get_error_line(&filename, &line_num) != 0)  {
On 2014/11/03 18:35:57, Kevin M wrote:
> On 2014/11/01 00:19:34, David Benjamin wrote:
> > On 2014/10/31 22:29:25, mark a. foltz wrote:
> > > Do we want to capture the return value from this to get the packed_error
> > integer
> > > from OpenSSL?
> > > 
> > > Even better, there are routines in err.c to turn them into human readable
> > > strings, which would probably help us understand what happened.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> > 
> > Please do not use ERR_error_string and use ERR_error_string_n instead.
> > ERR_error_string is not thread-safe. (On that note, I should excise its use
> from
> > net...)
> 
> ? The code is calling ERR_get_error_line(), which does not have a deprecation
> notice in err.h? Does it also have thread safety issues?

Nah, that was fine. Sorry, I was referring to the codesearch link. Although,
reading it again, it's specifically the buf == NULL case that's a problem.
(Though ERR_error_string_n is better for being explicit.)

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.h:63: const std::string&
peer_cert);
Nit: Why reformat this one?

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right):

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:338: static void
MangleString(std::string* str) {
Nit: Maybe worth a
  CHECK(!str->empty());

[mark a. foltz, 2014-11-03 19:06:27]

lgtm % some minor comments.

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

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:89: VLOG(1) <<
result.error_message;
Perhaps the caller should have this log statement instead of repeating it for
every early return (assuming there is one call site).

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.h:33:
ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT,
Omit trailing comma

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right):

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:311: class
CastSocketOpenSSLTest : public testing::Test {
CastAuthUtilTest

[Kevin M, 2014-11-03 19:41:56]

Also made changes to BUILD.gn.

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util_openssl.cc (right):

https://codereview.chromium.org/687733004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util_openssl.cc:135: if (!ctx) {
On 2014/11/01 00:19:35, David Benjamin wrote:
> 135 through 150 can be coalesced into one block now that you're capturing all
> the errors anyway.

Done.

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

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:89: VLOG(1) <<
result.error_message;
On 2014/11/03 19:06:27, mark a. foltz wrote:
> Perhaps the caller should have this log statement instead of repeating it for
> every early return (assuming there is one call site).

Huh, that is already the case. Removed the redundant VLOGs.

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.h:33:
ERROR_UNEXPECTED_AUTH_LIBRARY_RESULT,
On 2014/11/03 19:06:27, mark a. foltz wrote:
> Omit trailing comma

Done.

https://codereview.chromium.org/687733004/diff/120001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.h:63: const std::string&
peer_cert);
On 2014/11/03 19:01:50, David Benjamin wrote:
> Nit: Why reformat this one?

I think I renamed it to something else previously and reformatted around that -
switched back to the previous format.

[Kevin M, 2014-11-03 20:08:32]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2014-11-03 20:09:37]

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

[Kevin M, 2014-11-03 22:45:49]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2014-11-03 22:46:27]

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

[commit-bot: I haz the power, 2014-11-03 23:45:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-03 23:46:27]

Patchset 10 (id:??) landed as
https://crrev.com/311d5b8414d195abf8346b48900661503b83be57
Cr-Commit-Position: refs/heads/master@{#302520}