Gracefully handle NULL CertVerifiers in SSLClientSocket.

Gracefully handle NULL CertVerifiers in SSLClientSocket.

remoting does this. It's in the sandbox, so X509Certificate handles usually
aren't createable anyway. Explicitly check for this and return ERR_CERT_INVALID
rather than crash should that not happen on accident.

BUG=none

[davidben, 2015-04-23 00:57:12]

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

[davidben, 2015-04-23 00:57:12]

We can also yell at remoting, but I suspect everyone in the sandbox does this.
The context's constructor leaves it at null, and the sandbox doesn't let
X509Certificates be created most of the time anyway (grumble OSCertHandle
grumble), so it's kinda silly. Probably ought to just explicitly make it work.

[Ryan Sleevi, 2015-04-23 01:23:19]

On 2015/04/23 00:57:12, David Benjamin (OOO sick) wrote:
> We can also yell at remoting, but I suspect everyone in the sandbox does this.
> The context's constructor leaves it at null, and the sandbox doesn't let
> X509Certificates be created most of the time anyway (grumble OSCertHandle
> grumble), so it's kinda silly. Probably ought to just explicitly make it work.

I considered this, but explicitly requested fixing the code for remoting.

I'm not a fan of optional structures. If someone wants "do nothing" behaviour,
I'm much keener to make that explicit. I find it much harder to reason about
code with null-optionality, and obviously it creates issues when code flow is
changed because the conditions must always be extended.

[davidben, 2015-04-23 01:33:39]

On 2015/04/23 01:23:19, Ryan Sleevi wrote:
> On 2015/04/23 00:57:12, David Benjamin (OOO sick) wrote:
> > We can also yell at remoting, but I suspect everyone in the sandbox does
this.
> > The context's constructor leaves it at null, and the sandbox doesn't let
> > X509Certificates be created most of the time anyway (grumble OSCertHandle
> > grumble), so it's kinda silly. Probably ought to just explicitly make it
work.
> 
> I considered this, but explicitly requested fixing the code for remoting.
> 
> I'm not a fan of optional structures. If someone wants "do nothing" behaviour,
> I'm much keener to make that explicit. I find it much harder to reason about
> code with null-optionality, and obviously it creates issues when code flow is
> changed because the conditions must always be extended.

I'm not a fan either, but what would you suggest? They use IsAllowedBadCert to
let one certificate through. It ought to be a dummy CertVerifier that only
allows that one certificate, but that doesn't work either. OSCertHandle means
that we can't reliably get to that part of the code since it runs inside the
sandbox. I could stuff in a CertVerifier that just rejects everything outright.
It seems mildly silly though.

[Ryan Sleevi, 2015-04-23 01:44:27]

On 2015/04/23 01:33:39, David Benjamin (OOO sick) wrote:
> I could stuff in a CertVerifier that just rejects everything outright.
> It seems mildly silly though.

Sorry, yes, that's actually what I think of the fix as.

I mean, it seems logically/internally consistent with your remarks that if
OSCertHandle (or whatever) _was_ viable within the sandbox, then that's what
this code would have done all along - supply a CertVerifier that explicitly
accepts the expected peer cert and rejects all else, since they know the peer
cert apriori.

It also then simplifies what the internal //net contract needs to be, since now
there's "always a cert verifier". The fact that you had to test multiple times
of a cert_verifier in the other CL is proof of the logical complexity (each of
those checks adds branches to the state machine executing, and botching one =
one way trip to crashy town)

[davidben, 2015-04-23 18:41:51]

(Closing this one since the new version isn't going to share any code anyway.)