Only use the platform cert in verification in SSLClientSocketOpenSSL.

Only use the platform cert in verification in SSLClientSocketOpenSSL.

Align with SSLClientSocketNSS's behavior around the PeerCertificate object.
Within the sandbox for Chromoting, the platform certificate may be unavailable
while the SSL-layer certificate is. Match NSS in using the SSL-layer
certificate for many operations. Also check appropriately for the platform
certificate being NULL in DoVerifyCert rather than simply crashing.

BUG=414315
TEST=Install Chromoting app. Connecting to another computer works.

Committed: https://crrev.com/30798ed89c20e306765869a22f7d318daa209fed
Cr-Commit-Position: refs/heads/master@{#295764}

[davidben, 2014-09-17 20:10:54]

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

[davidben, 2014-09-17 20:10:55]

Compare against NSS uses of server_cert and server_cert_chain. Tested manually
with Chromoting and https://scripts.mit.edu/__scripts/certerror (my usual test
page for something that does renego). agl has taken down his triple-handshake
test, so I hacked up s3_clnt.c to parse the server Certificate wrong (notably it
pretends the intermediate is the leaf) which seems to still reject the renego
correctly.

(...we really need a way to test renego. Or just declare that we're not
supporting it anymore, which would be pretty wonderful.)

[Ryan Sleevi, 2014-09-18 01:34:39]

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:139: bool GetDEREncodedX509(X509* x509,
std::string* out_der) {
This seems to defeat the purpose of
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...

I'm wondering whether that layer should move into the common x509_util_openssl
from x509_certificate_openssl. Thoughts?

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:298: STACK_OF(X509)* chain) {
You (accidentally?) nuked a NULL check & ability to Reset(NULL).

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1037: // the cert status.
Don't use we in comments

// If the certificate is bad and has been previously accepted, use the previous
status and bypass
// the error.

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1053: // running inside sandbox.
// When running in a sandbox, it may not be possible to create an
X509Certificate*, as that may
// depend on OS functionality blocked in the sandbox.

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1620: if (server_cert_chain_->size() <
1) {
.empty() optimization?

[davidben, 2014-09-18 19:40:06]

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:139: bool GetDEREncodedX509(X509* x509,
std::string* out_der) {
On 2014/09/18 01:34:39, Ryan Sleevi wrote:
> This seems to defeat the purpose of
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...
> 
> I'm wondering whether that layer should move into the common x509_util_openssl
> from x509_certificate_openssl. Thoughts?

Hrm, that's a thought. It does bother me that we have a lot of handy logic for
X509s that are only available in USE_OPENSSL_CERTS ports. Done.

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:298: STACK_OF(X509)* chain) {
On 2014/09/18 01:34:39, Ryan Sleevi wrote:
> You (accidentally?) nuked a NULL check & ability to Reset(NULL).

I checked and X509_chain_up_ref will handle a NULL input properly. But it's
probably better to be explicit here, so I added a check.

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1037: // the cert status.
On 2014/09/18 01:34:39, Ryan Sleevi wrote:
> Don't use we in comments
> 
> // If the certificate is bad and has been previously accepted, use the
previous
> status and bypass
> // the error.

Done. (I just pulled this from SSLClientSocketNSS.)

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1053: // running inside sandbox.
On 2014/09/18 01:34:39, Ryan Sleevi wrote:
> // When running in a sandbox, it may not be possible to create an
> X509Certificate*, as that may
> // depend on OS functionality blocked in the sandbox.

Done. (Also just pulled from NSS.)

https://codereview.chromium.org/576233002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1620: if (server_cert_chain_->size() <
1) {
On 2014/09/18 01:34:39, Ryan Sleevi wrote:
> .empty() optimization?

Added a empty() method (matches NSS too) if that's what you meant.

[Ryan Sleevi, 2014-09-19 15:56:54]

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
File net/cert/x509_util_openssl.h (right):

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
net/cert/x509_util_openssl.h:49: 
Why does this need to be exported? It is just a wrapper for i2d, which no one
should need, right? Or they can use the X509Certificate method, which this seems
to be redundant with?

That is, why wouldn't you keep this method purely in the .cc, and make
GetDerAndCacheIfNeeded called GetDER. The fact that there is a cache is an
opaque detail, right?

[davidben, 2014-09-19 16:53:40]

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
File net/cert/x509_util_openssl.h (right):

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
net/cert/x509_util_openssl.h:49: 
On 2014/09/19 15:56:54, Ryan Sleevi wrote:
> Why does this need to be exported? It is just a wrapper for i2d, which no one
> should need, right? Or they can use the X509Certificate method, which this
seems
> to be redundant with?
> 
> That is, why wouldn't you keep this method purely in the .cc, and make
> GetDerAndCacheIfNeeded called GetDER. The fact that there is a cache is an
> opaque detail, right?

Well, that there is a cache isn't entirely opaque for a raw X509 since they're
not necessarily immutable, so I figured it'd be worth exposing both cached and
uncached versions. (GetDER is uncached.) I can unexpose GetDER if you prefer; I
think nothing outside x509_util_openssl.cc is actually using it.

This isn't redundant with the X509Certificate version because that's for the
platform one (which was the whole point of moving it to x509_util).

[Ryan Sleevi, 2014-09-19 17:39:33]

LGTM with the move to .cc and rename of AndCacheIfNeeded to just GetDER

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
File net/cert/x509_util_openssl.h (right):

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
net/cert/x509_util_openssl.h:49: 
On 2014/09/19 16:53:40, David Benjamin wrote:
> On 2014/09/19 15:56:54, Ryan Sleevi wrote:
> > Why does this need to be exported? It is just a wrapper for i2d, which no
one
> > should need, right? Or they can use the X509Certificate method, which this
> seems
> > to be redundant with?
> > 
> > That is, why wouldn't you keep this method purely in the .cc, and make
> > GetDerAndCacheIfNeeded called GetDER. The fact that there is a cache is an
> > opaque detail, right?
> 
> Well, that there is a cache isn't entirely opaque for a raw X509 since they're
> not necessarily immutable, so I figured it'd be worth exposing both cached and
> uncached versions. (GetDER is uncached.) I can unexpose GetDER if you prefer;
I
> think nothing outside x509_util_openssl.cc is actually using it.
> 

Yeah, I would rather wait until we have a demonstrable need for it.

> This isn't redundant with the X509Certificate version because that's for the
> platform one (which was the whole point of moving it to x509_util).

I was more assuming that the only time you'd want to get the DER for an X509 AND
explicitly not cache it would be on USE_OPENSSL_CERTS, which is why it seemed
redundant/unnecessary, and that everyone else should always be using the
AndCacheIfNeeded version.

[davidben, 2014-09-19 18:10:09]

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
File net/cert/x509_util_openssl.h (right):

https://codereview.chromium.org/576233002/diff/60001/net/cert/x509_util_opens...
net/cert/x509_util_openssl.h:49: 
On 2014/09/19 17:39:33, Ryan Sleevi wrote:
> On 2014/09/19 16:53:40, David Benjamin wrote:
> > On 2014/09/19 15:56:54, Ryan Sleevi wrote:
> > > Why does this need to be exported? It is just a wrapper for i2d, which no
> one
> > > should need, right? Or they can use the X509Certificate method, which this
> > seems
> > > to be redundant with?
> > > 
> > > That is, why wouldn't you keep this method purely in the .cc, and make
> > > GetDerAndCacheIfNeeded called GetDER. The fact that there is a cache is an
> > > opaque detail, right?
> > 
> > Well, that there is a cache isn't entirely opaque for a raw X509 since
they're
> > not necessarily immutable, so I figured it'd be worth exposing both cached
and
> > uncached versions. (GetDER is uncached.) I can unexpose GetDER if you
prefer;
> I
> > think nothing outside x509_util_openssl.cc is actually using it.
> > 
> 
> Yeah, I would rather wait until we have a demonstrable need for it.
> 
> > This isn't redundant with the X509Certificate version because that's for the
> > platform one (which was the whole point of moving it to x509_util).
> 
> I was more assuming that the only time you'd want to get the DER for an X509
AND
> explicitly not cache it would be on USE_OPENSSL_CERTS, which is why it seemed
> redundant/unnecessary, and that everyone else should always be using the
> AndCacheIfNeeded version.

Done.

[davidben, 2014-09-19 19:03:41]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-09-19 19:04:30]

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

[commit-bot: I haz the power, 2014-09-19 19:28:29]

Committed patchset #5 (id:80001) as 899a36a123075539d6244caf401bb98b506c9c8d

[commit-bot: I haz the power, 2014-09-19 19:32:14]

Patchset 5 (id:??) landed as
https://crrev.com/30798ed89c20e306765869a22f7d318daa209fed
Cr-Commit-Position: refs/heads/master@{#295764}