Porting SecureSocket to use BoringSSL on OSX

Ported SecureSocket to use BoringSSL on OSX and iOS. This included registering a callback with BoringSSL using SSL_CTX_set_cert_verify_callback, which overrides the BoringSSL certificate verification process, in order to verify certificates against the system's root certificates and then proceed to let BoringSSL handle the rest of the SSL session. In addition, this change includes refactoring to share BoringSSL code that used on all platforms.

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/644862bf966e6c079460258807ab3364b7e3759a

[bkonyi, 2017-05-25 22:40:12]

Description was changed from

==========
Porting SecureSocket to use BoringSSL on OSX

BUG=

patch from issue 2854203002 at patchset 20001
(http://crrev.com/2854203002#ps20001)
==========

to

==========
Porting SecureSocket to use BoringSSL on OSX

BUG=
==========

[bkonyi, 2017-05-25 22:44:37]

Description was changed from

==========
Porting SecureSocket to use BoringSSL on OSX

BUG=
==========

to

==========
Ported SecureSocket to use BoringSSL on OSX and iOS. This included registering a
callback with BoringSSL using SSL_CTX_set_cert_verify_callback, which overrides
the BoringSSL certificate verification process, in order to verify certificates
against the system's root certificates and then proceed to let BoringSSL handle
the rest of the SSL session. In addition, this change includes refactoring to
share BoringSSL code that used on all platforms.

BUG=
==========

[bkonyi, 2017-05-25 22:49:19]

bkonyi@google.com changed reviewers:
+ asiva@google.com, zra@google.com

[bkonyi, 2017-05-25 22:49:20]

All standalone/io tests pass on Linux and OSX, and HttpClient has no errors when
making a request to https://google.com on iOS. Pretty sure I caught all the
formatting issues, but I might have missed some as this is a large change. PTAL
when you have a chance.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:63: extern int CertificateCallback(int
preverify_ok, X509_STORE_CTX* store_ctx);
I'm not sure if this is what we should do, or if we should place the declaration
in a header somewhere. Thoughts?

[zra, 2017-05-26 18:11:14]

first round of comments.

still thinking about how to handle macos/ios. going to look at the code a bit
more, and see if I can think of something.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (left):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:1307: ASSERT(bad_certificate_callback_ == NULL);
Why remove this assert?

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:63: extern int CertificateCallback(int
preverify_ok, X509_STORE_CTX* store_ctx);
On 2017/05/25 22:49:20, bkonyi wrote:
> I'm not sure if this is what we should do, or if we should place the
declaration
> in a header somewhere. Thoughts?

Yes, try to put the declaration in a header.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:160: #if !defined(HOST_OS_MACOS)
Let's not have platform #ifdefs in the non-platform-specific files.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:26: #include <openssl/bio.h>
These should go after platform/globals.h but before the includes under bin/

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:43: void ThrowIOException(int status,
I'd make these statics of an SSLUtils class.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:53: void CheckStatus(int status, const char* type,
const char* message);
"CheckStatus" in particular is probably not a great top-level name to introduce
into the dart::bin namespace

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:117: // TODO(whesse): make private:
Maybe now is a good time to try to take care of this TODO.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:140: static bool isBufferEncrypted(int i) {
IsBufferEncrypted

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_ios.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:112: SecCertificateRef
CreateMacosCertFromX509(X509* cert) {
MacosCert seems a little off to me since the return type says you're making a
SecCertificate. Maybe: MacosCert -> SecCertificate so it would be
CreateSecCertificateFromX509.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:129: int
MacosCertVerificationCallback(X509_STORE_CTX* ctx, void* arg) {
static

Maybe just CertVerificationCallback

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:143: ASSERT(cert != NULL);
Verification should probably fail if we can't parse the cert.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:215: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx) {
static

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:271: SecCertificateRef peer =
CreateMacosCertFromX509(ca);
NULL check?

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_ios.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:88: CFArrayRef cert_chain() { return
cert_chain_; }
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:98: CFArrayRef trusted_certs() { return
trusted_certs_; }
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:108: CFArrayRef cert_authorities() { return
cert_authorities_; }
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:118: bool trust_builtin() {
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_macos.cc (left):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:4: 
restore newline

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:48: extern const int
kSSLFilterNativeFieldIndex;
Why are these extern? Maybe make them static const fields of SSLFilter and
SSLCertContext respectively.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:51: const int kX509NativeFieldIndex = 0;
This one could go in the SSLCertContext, too, I think.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:120: static void CheckStatusMacOS(int status,
I think you can call this just CheckStatus if you move the other CheckStatus
function into a utility class.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:130: SecCertificateRef
CreateMacosCertFromX509(X509* cert) {
Same comment about MacosCert -> SecCertificate.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:147: int
MacosCertVerificationCallback(X509_STORE_CTX* ctx, void* arg) {
static

Maybe just CertVerificationCallback

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:161: ASSERT(cert != NULL);
Same comment here as in _ios.cc

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:233: int CertificateCallback(int
preverify_ok, X509_STORE_CTX* store_ctx) {
static

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:496: static bool NoPEMStartLine() {
Maybe move to a utility class in secure_socket.h

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_macos.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:87: CFArrayRef cert_chain() {
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:99: CFArrayRef trusted_certs() {
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:111: CFArrayRef cert_authorities() {
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:123: bool trust_builtin() {
const

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_scope_utils.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_scope_utils.h:1: // Copyright (c) 2017, the Dart
project authors.  Please see the AUTHORS file
Maybe just secure_socket_utils.h and put some of the other utility methods in
here, too.

[kevmoo, 2017-05-26 19:17:01]

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

[kevmoo, 2017-05-26 19:17:01]

Will we be able to use ALPN on mac w/ this? Per
https://github.com/dart-lang/pub-dartlang-dart/issues/80

CC Martin

[zra, 2017-05-26 19:28:31]

On 2017/05/26 19:17:01, kevmoo wrote:
> Will we be able to use ALPN on mac w/ this? Per
> https://github.com/dart-lang/pub-dartlang-dart/issues/80
> 
> CC Martin

Yes.

[kevmoo, 2017-05-26 19:33:05]

On 2017/05/26 19:28:31, zra wrote:
> On 2017/05/26 19:17:01, kevmoo wrote:
> > Will we be able to use ALPN on mac w/ this? Per
> > https://github.com/dart-lang/pub-dartlang-dart/issues/80
> > 
> > CC Martin
> 
> Yes.

Woo hoo!

[kevmoo, 2017-05-26 19:33:26]

Please update changelog accordingly? Guessing this will be 1.25...

[zra, 2017-05-26 19:36:11]

On 2017/05/26 19:33:26, kevmoo wrote:
> Please update changelog accordingly? Guessing this will be 1.25...

I'd advise updating the changelog in a separate CL after the change lands and
sticks.

[bkonyi, 2017-05-26 23:35:31]

I've addressed the comments by zra@ and fixed some issues. PTAL when you have a
chance!

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (left):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:1307: ASSERT(bad_certificate_callback_ == NULL);
On 2017/05/26 18:11:12, zra wrote:
> Why remove this assert?

Probably did it during all the refactoring and didn't realize... added back.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:63: extern int CertificateCallback(int
preverify_ok, X509_STORE_CTX* store_ctx);
On 2017/05/26 18:11:12, zra wrote:
> On 2017/05/25 22:49:20, bkonyi wrote:
> > I'm not sure if this is what we should do, or if we should place the
> declaration
> > in a header somewhere. Thoughts?
> 
> Yes, try to put the declaration in a header.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:160: #if !defined(HOST_OS_MACOS)
On 2017/05/26 18:11:12, zra wrote:
> Let's not have platform #ifdefs in the non-platform-specific files.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:26: #include <openssl/bio.h>
On 2017/05/26 18:11:12, zra wrote:
> These should go after platform/globals.h but before the includes under bin/

cpplint will complain if we do that. I get this output when trying to upload the
CL: "Found C system header after other header. Should be: secure_socket.h, c
system, c++ system, other.  [build/include_order] [4]"

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:43: void ThrowIOException(int status,
On 2017/05/26 18:11:12, zra wrote:
> I'd make these statics of an SSLUtils class.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:53: void CheckStatus(int status, const char* type,
const char* message);
On 2017/05/26 18:11:12, zra wrote:
> "CheckStatus" in particular is probably not a great top-level name to
introduce
> into the dart::bin namespace

Yeah, I wasn't sure about this. I'll add it to the utils class.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:117: // TODO(whesse): make private:
On 2017/05/26 18:11:12, zra wrote:
> Maybe now is a good time to try to take care of this TODO.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.h:140: static bool isBufferEncrypted(int i) {
On 2017/05/26 18:11:12, zra wrote:
> IsBufferEncrypted

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_ios.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:112: SecCertificateRef
CreateMacosCertFromX509(X509* cert) {
On 2017/05/26 18:11:12, zra wrote:
> MacosCert seems a little off to me since the return type says you're making a
> SecCertificate. Maybe: MacosCert -> SecCertificate so it would be
> CreateSecCertificateFromX509.

Yes, that makes more sense. Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:129: int
MacosCertVerificationCallback(X509_STORE_CTX* ctx, void* arg) {
On 2017/05/26 18:11:12, zra wrote:
> static
> 
> Maybe just CertVerificationCallback

I'll go with CertificateVerificationCallback to be consistent with
CertificateCallback. Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:143: ASSERT(cert != NULL);
On 2017/05/26 18:11:12, zra wrote:
> Verification should probably fail if we can't parse the cert.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:215: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx) {
On 2017/05/26 18:11:12, zra wrote:
> static

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.cc:271: SecCertificateRef peer =
CreateMacosCertFromX509(ca);
On 2017/05/26 18:11:12, zra wrote:
> NULL check?

I've added a check to see if peer == NULL. At this point, we really shouldn't
have issues parsing the cert since we should have already done it at some point
during the certificate verification callback. Should this be an ASSERT instead?

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_ios.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:88: CFArrayRef cert_chain() { return
cert_chain_; }
On 2017/05/26 18:11:12, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:98: CFArrayRef trusted_certs() { return
trusted_certs_; }
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:108: CFArrayRef cert_authorities() { return
cert_authorities_; }
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_ios.h:118: bool trust_builtin() {
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_macos.cc (left):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:4: 
On 2017/05/26 18:11:13, zra wrote:
> restore newline

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:48: extern const int
kSSLFilterNativeFieldIndex;
On 2017/05/26 18:11:13, zra wrote:
> Why are these extern? Maybe make them static const fields of SSLFilter and
> SSLCertContext respectively.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:51: const int kX509NativeFieldIndex = 0;
On 2017/05/26 18:11:13, zra wrote:
> This one could go in the SSLCertContext, too, I think.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:120: static void CheckStatusMacOS(int status,
On 2017/05/26 18:11:13, zra wrote:
> I think you can call this just CheckStatus if you move the other CheckStatus
> function into a utility class.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:130: SecCertificateRef
CreateMacosCertFromX509(X509* cert) {
On 2017/05/26 18:11:13, zra wrote:
> Same comment about MacosCert -> SecCertificate.

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:147: int
MacosCertVerificationCallback(X509_STORE_CTX* ctx, void* arg) {
On 2017/05/26 18:11:13, zra wrote:
> static
> 
> Maybe just CertVerificationCallback

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:161: ASSERT(cert != NULL);
On 2017/05/26 18:11:13, zra wrote:
> Same comment here as in _ios.cc

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:233: int CertificateCallback(int
preverify_ok, X509_STORE_CTX* store_ctx) {
On 2017/05/26 18:11:13, zra wrote:
> static

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.cc:496: static bool NoPEMStartLine() {
On 2017/05/26 18:11:13, zra wrote:
> Maybe move to a utility class in secure_socket.h

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_macos.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:87: CFArrayRef cert_chain() {
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:99: CFArrayRef trusted_certs() {
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:111: CFArrayRef cert_authorities() {
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_macos.h:123: bool trust_builtin() {
On 2017/05/26 18:11:13, zra wrote:
> const

Done.

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket_scope_utils.h (right):

https://codereview.chromium.org/2903743002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket_scope_utils.h:1: // Copyright (c) 2017, the Dart
project authors.  Please see the AUTHORS file
On 2017/05/26 18:11:13, zra wrote:
> Maybe just secure_socket_utils.h and put some of the other utility methods in
> here, too.

Done.

[zra, 2017-05-30 16:15:37]

I think we should consider splitting the filter code and entry points, and the
security context code and entry points into different files. Something like:

secure_socket_filter.h
secure_socket_filter.cc  // This should all be platform generic, right?
security_context.h
security_context.cc
security_context_macos.h
security_context_macos.cc
security_context_ios.h
security_context_ios.cc
security_context_boringssl.h
security_context_boringssl.cc

After doing this, I'd be interested in knowing how big the diff is between
_macos.{cc,h} and _ios.{cc,h}.

https://codereview.chromium.org/2903743002/diff/120001/runtime/bin/secure_soc...
File runtime/bin/secure_socket.cc (left):

https://codereview.chromium.org/2903743002/diff/120001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:781: void
FUNCTION_NAME(SecurityContext_SetTrustedCertificatesBytes)(
To be consistent with other parts of the dart:io implementation, the platform
agnostic file (like this one) should have the runtime entry points (like
SecurityContext_SetTrustedCertificatesBytes()), and the platform specific files
should have the implementation. It wasn't like this before because *all* of the
entry points were duplicated, but it's probably confusing to have some entry
points duplicated and some not. This refactor should allow cleaning up/de-duping
the error handling code a bit.

https://codereview.chromium.org/2903743002/diff/120001/runtime/bin/secure_soc...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/2903743002/diff/120001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:88: void SecureSocketUtils::ThrowIOException(int
status,
Please keep the SecureSocketUtils methods all together, or put them in their own
secure_socket_utils.cc file.

[zra, 2017-05-30 16:18:33]

Oh, also, this CL should enable setting the trusted certs on the command line
with:

https://github.com/dart-lang/sdk/blob/master/runtime/bin/main.cc#L604

[bkonyi, 2017-06-02 21:22:53]

I've managed to remove most of the platform specific code, so hopefully that
makes things a bit easier to review. PTAL when you have a chance!

[kevmoo, 2017-06-02 22:17:19]

Is "SecurityContext.alpnSupported" true for all platforms now?

This change should be documented in the CHANGELOG.

Should we mark this API as deprecated now (assuming it is always true...)

[zra, 2017-06-02 22:54:30]

On 2017/06/02 22:17:19, kevmoo wrote:
> Is "SecurityContext.alpnSupported" true for all platforms now?
> 
> This change should be documented in the CHANGELOG.
> 
> Should we mark this API as deprecated now (assuming it is always true...)

This change is already sort of complicated, so let's handle the API change and
the CHANGELOG update in a follow-up CL.

[zra, 2017-06-02 22:56:24]

This is looking much better =)

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/io_impl_so...
File runtime/bin/io_impl_sources.gypi (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/io_impl_so...
runtime/bin/io_impl_sources.gypi:61: 'security_context.cc',
Do you need security_context_unsupported.cc?

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket.h:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
I'd just delete this file, and move its contents to secure_socket_filter.h.
Include both secure_socket_filter.h and security_context.h where this was
included before, like in bin/io_service.cc.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket.h:17: const bool SSL_LOG_STATUS = false;
Probably move these to secure_socket_utils.h

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.cc (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.cc:13: #include "platform/globals.h"
Is this needed?

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.cc:589: int printErrorCallback(const char* str,
size_t len, void* ctx) {
Is this used?

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.h (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:12: #include "platform/globals.h"
Is this header file needed?

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:95: void RegisterCallbacks(SSLCertContext*
cert_ctx);
Can this be a public method on SSLCertContext? You'd call it from
SSLFilter::Connect as before, and pass ssl_.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_utils.h (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.h:52: class X509Helper : public AllStatic {
This is only used in security_context.cc. Maybe move this declaration to
security_context.h.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
File runtime/bin/security_context_boringssl.cc (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
runtime/bin/security_context_boringssl.cc:79: #if defined(HOST_OS_ANDROID)
This is looking really good.

At this point I think it would be reasonable to do the following:
1. Add AddCompiledInCerts to SSLCertContext, moving the code to
security_context.cc
2. Make e.g. security_context_{android, linux, fuchsia, windows}.cc containing
SSLCertContext::TrustBuiltinRoots() and SSLCertContext::RegisterCallbacks().
This is because it's likely that we're going to have more platform-specific code
for windows and Fuchsia in the future.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
File runtime/bin/security_context_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
runtime/bin/security_context_macos.cc:102: CFRelease(policy);
Maybe a scope class would be useful here so that we don't forget to release
something:

// Pseudo-code. No guarantees that this is right.

class CFTypeRefScope {
 public:
  explicit CFTypeRefScope(CFTypeRef ref) : ref_(ref) {}
  ~CFTypeRefScope() { CFRelease(ref_); }

 private:
  CFTypeRef ref_;

  DISALLOW_ALLOCATION();
  DISALLOC_COPY_AND_ASSIGN(CFTypeRefScope);
};

If you are feeling extra fancy, you could template it on the actual CF type...

[bkonyi, 2017-06-05 20:25:52]

Addressed most of Zach's comments. PTAL when you have a chance.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/io_impl_so...
File runtime/bin/io_impl_sources.gypi (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/io_impl_so...
runtime/bin/io_impl_sources.gypi:61: 'security_context.cc',
On 2017/06/02 22:56:24, zra wrote:
> Do you need security_context_unsupported.cc?

secure_socket_unsupported.cc contains all of the entry points already, so this
should still work fine. However, I can split secure_socket_unsupported.cc into
secure_socket_filter_unsupported.cc and security_context_unsupported.cc if that
makes more sense.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket.h:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
On 2017/06/02 22:56:24, zra wrote:
> I'd just delete this file, and move its contents to secure_socket_filter.h.
> Include both secure_socket_filter.h and security_context.h where this was
> included before, like in bin/io_service.cc.

Completely missed this comment. Will update in next patch.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket.h:17: const bool SSL_LOG_STATUS = false;
On 2017/06/02 22:56:24, zra wrote:
> Probably move these to secure_socket_utils.h

Done.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.cc (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.cc:13: #include "platform/globals.h"
On 2017/06/02 22:56:24, zra wrote:
> Is this needed?

Nope. Removed.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.cc:589: int printErrorCallback(const char* str,
size_t len, void* ctx) {
On 2017/06/02 22:56:24, zra wrote:
> Is this used?

Nope. Removed.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.h (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:12: #include "platform/globals.h"
On 2017/06/02 22:56:24, zra wrote:
> Is this header file needed?

Apparently not. Removed.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:95: void RegisterCallbacks(SSLCertContext*
cert_ctx);
On 2017/06/02 22:56:24, zra wrote:
> Can this be a public method on SSLCertContext? You'd call it from
> SSLFilter::Connect as before, and pass ssl_.

Done.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_utils.h (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.h:52: class X509Helper : public AllStatic {
On 2017/06/02 22:56:24, zra wrote:
> This is only used in security_context.cc. Maybe move this declaration to
> security_context.h.

Done.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
File runtime/bin/security_context_boringssl.cc (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
runtime/bin/security_context_boringssl.cc:79: #if defined(HOST_OS_ANDROID)
On 2017/06/02 22:56:24, zra wrote:
> This is looking really good.
> 
> At this point I think it would be reasonable to do the following:
> 1. Add AddCompiledInCerts to SSLCertContext, moving the code to
> security_context.cc
> 2. Make e.g. security_context_{android, linux, fuchsia, windows}.cc containing
> SSLCertContext::TrustBuiltinRoots() and SSLCertContext::RegisterCallbacks().
> This is because it's likely that we're going to have more platform-specific
code
> for windows and Fuchsia in the future.

Done.

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
File runtime/bin/security_context_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/180001/runtime/bin/security_c...
runtime/bin/security_context_macos.cc:102: CFRelease(policy);
On 2017/06/02 22:56:24, zra wrote:
> Maybe a scope class would be useful here so that we don't forget to release
> something:
> 
> // Pseudo-code. No guarantees that this is right.
> 
> class CFTypeRefScope {
>  public:
>   explicit CFTypeRefScope(CFTypeRef ref) : ref_(ref) {}
>   ~CFTypeRefScope() { CFRelease(ref_); }
> 
>  private:
>   CFTypeRef ref_;
> 
>   DISALLOW_ALLOCATION();
>   DISALLOC_COPY_AND_ASSIGN(CFTypeRefScope);
> };
> 
> If you are feeling extra fancy, you could template it on the actual CF type...

Done.

[zra, 2017-06-05 21:06:31]

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.h (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:24: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx);
Maybe move this to a private static on SSLCertContext.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:41: static Dart_Handle
WrappedX509Certificate(X509* certificate);
Maybe move to X509Helper

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:92: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx);
It looks like this is not defined.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_utils.cc (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.cc:96: }  // namespace dart
Need a newline here

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.cc:97: #endif
// !defined(DART_IO_DISABLED) && !defined(DART_IO_SECURE_SOCKET_DISABLED)

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
File runtime/bin/security_context.cc (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
runtime/bin/security_context.cc:764: }  // namespace dart
Missing newline and comment after endif.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
File runtime/bin/security_context.h (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
runtime/bin/security_context.h:98: DISALLOW_ALLOCATION();
AllStatic already adds these, I believe.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
File runtime/bin/security_context_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
runtime/bin/security_context_macos.cc:1: // Copyright (c) 2016, the Dart project
authors.  Please see the AUTHORS file
2017

[bkonyi, 2017-06-06 00:48:35]

PTAL when you have a chance.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.h (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:24: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx);
On 2017/06/05 21:06:30, zra wrote:
> Maybe move this to a private static on SSLCertContext.

I'm pretty sure I've tried, but on either Mac or Linux the compiler complains. I
think we have to keep it like this.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:41: static Dart_Handle
WrappedX509Certificate(X509* certificate);
On 2017/06/05 21:06:30, zra wrote:
> Maybe move to X509Helper

Done.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:92: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx);
On 2017/06/05 21:06:30, zra wrote:
> It looks like this is not defined.

It was at one point when I was trying to pull CertificateCallback into a class
without success. Removed.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_utils.cc (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.cc:96: }  // namespace dart
On 2017/06/05 21:06:31, zra wrote:
> Need a newline here

Done.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.cc:97: #endif
On 2017/06/05 21:06:31, zra wrote:
> // !defined(DART_IO_DISABLED) && !defined(DART_IO_SECURE_SOCKET_DISABLED)

Done.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
File runtime/bin/security_context.cc (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
runtime/bin/security_context.cc:764: }  // namespace dart
On 2017/06/05 21:06:31, zra wrote:
> Missing newline and comment after endif.

Done.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
File runtime/bin/security_context.h (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
runtime/bin/security_context.h:98: DISALLOW_ALLOCATION();
On 2017/06/05 21:06:31, zra wrote:
> AllStatic already adds these, I believe.

Done.

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
File runtime/bin/security_context_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/security_c...
runtime/bin/security_context_macos.cc:1: // Copyright (c) 2016, the Dart project
authors.  Please see the AUTHORS file
On 2017/06/05 21:06:31, zra wrote:
> 2017

Done.

[zra, 2017-06-06 03:09:39]

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.h (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:24: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx);
On 2017/06/06 00:48:34, bkonyi wrote:
> On 2017/06/05 21:06:30, zra wrote:
> > Maybe move this to a private static on SSLCertContext.
> 
> I'm pretty sure I've tried, but on either Mac or Linux the compiler complains.
I
> think we have to keep it like this.

It has to be public, but this worked for me on Linux. Let's take a look together
tomorrow.

[bkonyi, 2017-06-06 18:04:43]

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_filter.h (right):

https://codereview.chromium.org/2903743002/diff/220001/runtime/bin/secure_soc...
runtime/bin/secure_socket_filter.h:24: int CertificateCallback(int preverify_ok,
X509_STORE_CTX* store_ctx);
On 2017/06/06 03:09:39, zra wrote:
> On 2017/06/06 00:48:34, bkonyi wrote:
> > On 2017/06/05 21:06:30, zra wrote:
> > > Maybe move this to a private static on SSLCertContext.
> > 
> > I'm pretty sure I've tried, but on either Mac or Linux the compiler
complains.
> I
> > think we have to keep it like this.
> 
> It has to be public, but this worked for me on Linux. Let's take a look
together
> tomorrow.

You're right, that works. I must have been doing something else before... Done.

[zra, 2017-06-06 19:16:27]

lgtm w/ nits

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_utils.h (right):

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.h:51: DISALLOW_ALLOCATION();
These are inherited from AllStatic

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
File runtime/bin/security_context.h (right):

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
runtime/bin/security_context.h:19: class SSLFilter;
Is this needed?

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
File runtime/bin/security_context_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
runtime/bin/security_context_macos.cc:55: SecCertificateRef
CreateSecCertificateFromX509(X509* cert) {
static

[bkonyi, 2017-06-06 19:51:39]

Addressed nits and confirmed working on Windows.

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_utils.h (right):

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/secure_soc...
runtime/bin/secure_socket_utils.h:51: DISALLOW_ALLOCATION();
On 2017/06/06 19:16:27, zra wrote:
> These are inherited from AllStatic

Removed.

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
File runtime/bin/security_context.h (right):

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
runtime/bin/security_context.h:19: class SSLFilter;
On 2017/06/06 19:16:27, zra wrote:
> Is this needed?

Yes, since secure_socket_filter.h includes this file.

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
File runtime/bin/security_context_macos.cc (right):

https://codereview.chromium.org/2903743002/diff/260001/runtime/bin/security_c...
runtime/bin/security_context_macos.cc:55: SecCertificateRef
CreateSecCertificateFromX509(X509* cert) {
On 2017/06/06 19:16:27, zra wrote:
> static

Done.

[bkonyi, 2017-06-06 19:53:04]

Description was changed from

==========
Ported SecureSocket to use BoringSSL on OSX and iOS. This included registering a
callback with BoringSSL using SSL_CTX_set_cert_verify_callback, which overrides
the BoringSSL certificate verification process, in order to verify certificates
against the system's root certificates and then proceed to let BoringSSL handle
the rest of the SSL session. In addition, this change includes refactoring to
share BoringSSL code that used on all platforms.

BUG=
==========

to

==========
Ported SecureSocket to use BoringSSL on OSX and iOS. This included registering a
callback with BoringSSL using SSL_CTX_set_cert_verify_callback, which overrides
the BoringSSL certificate verification process, in order to verify certificates
against the system's root certificates and then proceed to let BoringSSL handle
the rest of the SSL session. In addition, this change includes refactoring to
share BoringSSL code that used on all platforms.

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/644862bf966e6c079460258807ab3364b7e3759a
==========

[bkonyi, 2017-06-06 19:53:06]

Committed patchset #15 (id:280001) manually as
644862bf966e6c079460258807ab3364b7e3759a (presubmit successful).