Adds SecurityContext.setTrustedCertificatesBytes

Adds SecurityContext.setTrustedCertificatesBytes

Implements SecurityContext.setTrustedCertificates in terms of the new
function. This requires setTrustedCertificates to return a Future,
and removing the `directory` named argument as it is not possible to
implement with BoringSSL without blocking IO.

R=whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/2cf6405ca9fe3f87d90ed75d015c960b78a49ce9

[zra, 2016-02-02 21:14:09]

Description was changed from

==========
Adds SecurityContext.setTrustedCertificatesBytes

Implements SecurityContext.setTrustedCertificates in terms of the new
function. This requires setTrustedCertificates to return a Future,
and removing the `directory` named argument as it is not possible to
implement without blocking IO.
==========

to

==========
Adds SecurityContext.setTrustedCertificatesBytes

Implements SecurityContext.setTrustedCertificates in terms of the new
function. This requires setTrustedCertificates to return a Future,
and removing the `directory` named argument as it is not possible to
implement with BoringSSL without blocking IO.
==========

[zra, 2016-02-02 21:14:24]

zra@google.com changed reviewers:
+ iposva@google.com, whesse@google.com

[Bill Hesse, 2016-02-02 23:17:32]

We don't know if we have users using the directory argument, but if so, they
will be totally out of luck.  This should be the standard way of adding a lot of
certificates, like custom roots, to a context.

If this is removed, will we have a method to add trusted certificates, or does
this method add them to the existing trusted certs, instead of replacing the
existing trusted certs?

[zra, 2016-02-03 07:06:59]

On 2016/02/02 23:17:32, Bill Hesse wrote:
> We don't know if we have users using the directory argument, but if so, they
> will be totally out of luck.  This should be the standard way of adding a lot
of
> certificates, like custom roots, to a context.

There isn't a good analog for the directory argument to
SSL_CTX_load_verify_locations() in the Mac API. If someone wants to simulate it
after it's removed, they can do:

#!/bin/sh
 rm CAfile.pem
 for i in ca1.pem ca2.pem ca3.pem ; do
   openssl x509 -in $i -text >> CAfile.pem
 done

to aggregate their certificates into a single file (from the docs at
https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_load_verify_locations.html).

We could do the same in the Dart code, I guess, but unlike the directory
argument to SSL_CTX_load_verify_locations(), the certificates would be read and
added to the context eagerly. Since that behavior is a bit different, I think it
would be better to remove the option entirely.

It also looks like we don't have any tests of the directory argument...

> If this is removed, will we have a method to add trusted certificates, or does
> this method add them to the existing trusted certs, instead of replacing the
> existing trusted certs?

This method still adds certificates to the existing trusted certificates.

If we wanted to, it is also possible to blow away a context's certificate store,
but AFAIU that isn't what SSL_CTX_load_verify_locations() was doing.

[Bill Hesse, 2016-02-04 18:54:36]

Sorry I didn't get to this today, but Søren asked today why all of these need to
be async implementations.  Why can't they use readAsBytesSync instead, and not
change their API to return a future.

Also, shouldn't a breaking change message go out about these API changes (if we
do need to make it a future), since these are now getting into dev channel?

[kevmoo, 2016-02-04 19:14:53]

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

[kevmoo, 2016-02-04 19:14:54]

This *is* a breaking change – for a feature we just added.

https://codereview.chromium.org/1665433002/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1665433002/diff/40001/CHANGELOG.md#newcode10
CHANGELOG.md:10: The non-`Bytes` methods now return a `Future`.
Please group these under 'dart:io" – see previous release.

Also: we should discuss this as a breaking change for 1.15

[zra, 2016-02-04 22:31:02]

On 2016/02/04 18:54:36, Bill Hesse wrote:
> Sorry I didn't get to this today, but Søren asked today why all of these need
to
> be async implementations.  Why can't they use readAsBytesSync instead, and not
> change their API to return a future.

That's fine with me as long as we rename the functions to be Sync themselves,
e.g. `SecurityContext.usePrivateKeySync`. I've gone ahead and made that change
in this CL.

> Also, shouldn't a breaking change message go out about these API changes (if
we
> do need to make it a future), since these are now getting into dev channel?

Do you have a link to the procedure for doing this? I've got one more CL after
this one to update `SecurityContext.setClientAuthorities` here:
https://codereview.chromium.org/1663633003/. Maybe we should wait until after
that one lands to send a message out.

[zra, 2016-02-04 22:31:41]

https://codereview.chromium.org/1665433002/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1665433002/diff/40001/CHANGELOG.md#newcode10
CHANGELOG.md:10: The non-`Bytes` methods now return a `Future`.
On 2016/02/04 19:14:53, kevmoo wrote:
> Please group these under 'dart:io" – see previous release.
> 
> Also: we should discuss this as a breaking change for 1.15

Done.

[zra, 2016-02-05 04:02:54]

Merged https://codereview.chromium.org/1663633003/ into here.

[Bill Hesse, 2016-02-05 14:48:18]

First comments, more in 4-5 hours.

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

https://codereview.chromium.org/1665433002/diff/140001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:379: static bool GetBIOArgument(Dart_Handle object,
BIO** bio) {
I would call this FillMemBIO.  "Argument" doesn't add anything, and the BIO
isn't being gotten.

https://codereview.chromium.org/1665433002/diff/140001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:413: 
This seems really fragile (the typed data release), but looking at its uses, I
guess it is needed.   Can we put comments that FreeBIO (FreeMemBIO) must be
called after a successful return from FillMemBIO, without any non-fatal returns
between the two calls.

[zra, 2016-02-05 17:18:22]

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

https://codereview.chromium.org/1665433002/diff/140001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:379: static bool GetBIOArgument(Dart_Handle object,
BIO** bio) {
On 2016/02/05 14:48:17, Bill Hesse wrote:
> I would call this FillMemBIO.  "Argument" doesn't add anything, and the BIO
> isn't being gotten.

Replaced these functions with MemBIOScope

https://codereview.chromium.org/1665433002/diff/140001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:413: 
On 2016/02/05 14:48:17, Bill Hesse wrote:
> This seems really fragile (the typed data release), but looking at its uses, I
> guess it is needed.   Can we put comments that FreeBIO (FreeMemBIO) must be
> called after a successful return from FillMemBIO, without any non-fatal
returns
> between the two calls.

Acknowledged.

[Bill Hesse, 2016-02-05 21:35:40]

LGTM, with comments.

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

https://codereview.chromium.org/1665433002/diff/160001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:397: ThrowIfError(Dart_TypedDataAcquireData(
Any idea what PropagateError from a constructor or destructor does?  Could be a
memory leak.  But this is quite minor - I think it is OK.

https://codereview.chromium.org/1665433002/diff/160001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:404: is_typed_data = false;
I wouldn't assign false to is_typed_data twice.

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
File tests/standalone/io/http_proxy_advanced_test.dart (right):

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
tests/standalone/io/http_proxy_advanced_test.dart:21:
..setTrustedCertificatesBytes(
This is exactly the same as the implementation of setTrustedCertificatesSync. 
Is there a benefit to making it explicit this way?

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
File tests/standalone/io/https_client_certificate_test.dart (right):

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
tests/standalone/io/https_client_certificate_test.dart:26:
..useCertificateChainBytes(readLocalFile('certificates/server_chain.pem'))
Same for here, this is the same as useCertificateChainSync.

[kevmoo, 2016-02-05 22:26:06]

I'm looking at the delta here from what's in 1.14

https://api.dartlang.org/1.14.1/dart-io/SecurityContext-class.html

We're going to break folks just to add a 'Sync' postfix on methods that are
clearly sync.

Why don't we just add a doc comment encouraging folks to use the byte-based APIs
and not break folks?

This change does not pay the tax of introducing a breaking change.

https://codereview.chromium.org/1665433002/diff/160001/sdk/lib/io/security_co...
File sdk/lib/io/security_context.dart (left):

https://codereview.chromium.org/1665433002/diff/160001/sdk/lib/io/security_co...
sdk/lib/io/security_context.dart:112: void setClientAuthorities(String file);
Keep this API around for 1.15 marked as deprecated and pointing to
setClientAuthoritiesSync

Is there a desired to ship an async version of this? Why change the name at all?

[zra, 2016-02-05 23:10:39]

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

https://codereview.chromium.org/1665433002/diff/160001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:397: ThrowIfError(Dart_TypedDataAcquireData(
On 2016/02/05 21:35:39, Bill Hesse wrote:
> Any idea what PropagateError from a constructor or destructor does?  Could be
a
> memory leak.  But this is quite minor - I think it is OK.

Checked with iposva offline. From the point of view of Dart_Propagate error, a
constructor is just like any other function call.

https://codereview.chromium.org/1665433002/diff/160001/runtime/bin/secure_soc...
runtime/bin/secure_socket.cc:404: is_typed_data = false;
On 2016/02/05 21:35:39, Bill Hesse wrote:
> I wouldn't assign false to is_typed_data twice.

Done.

https://codereview.chromium.org/1665433002/diff/160001/sdk/lib/io/security_co...
File sdk/lib/io/security_context.dart (left):

https://codereview.chromium.org/1665433002/diff/160001/sdk/lib/io/security_co...
sdk/lib/io/security_context.dart:112: void setClientAuthorities(String file);
On 2016/02/05 22:26:06, kevmoo wrote:
> Keep this API around for 1.15 marked as deprecated and pointing to
> setClientAuthoritiesSync

Done.

> Is there a desired to ship an async version of this? Why change the name at
all?

The functions are *not* obviously synchronous. I had to dig through the
BoringSSL code to discover that that is what was happening. The name without
Sync is wrong. It is a bug we have to fix.

We are shipping an async-friendly version. Those are the Bytes functions.

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
File tests/standalone/io/http_proxy_advanced_test.dart (right):

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
tests/standalone/io/http_proxy_advanced_test.dart:21:
..setTrustedCertificatesBytes(
On 2016/02/05 21:35:39, Bill Hesse wrote:
> This is exactly the same as the implementation of setTrustedCertificatesSync. 
> Is there a benefit to making it explicit this way?

Done.

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
File tests/standalone/io/https_client_certificate_test.dart (right):

https://codereview.chromium.org/1665433002/diff/160001/tests/standalone/io/ht...
tests/standalone/io/https_client_certificate_test.dart:26:
..useCertificateChainBytes(readLocalFile('certificates/server_chain.pem'))
On 2016/02/05 21:35:39, Bill Hesse wrote:
> Same for here, this is the same as useCertificateChainSync.

Done.

[Bill Hesse, 2016-02-08 09:24:50]

lgtm

[zra, 2016-02-08 16:41:52]

Description was changed from

==========
Adds SecurityContext.setTrustedCertificatesBytes

Implements SecurityContext.setTrustedCertificates in terms of the new
function. This requires setTrustedCertificates to return a Future,
and removing the `directory` named argument as it is not possible to
implement with BoringSSL without blocking IO.
==========

to

==========
Adds SecurityContext.setTrustedCertificatesBytes

Implements SecurityContext.setTrustedCertificates in terms of the new
function. This requires setTrustedCertificates to return a Future,
and removing the `directory` named argument as it is not possible to
implement with BoringSSL without blocking IO.

R=whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2cf6405ca9fe3f87d90ed75d015c960b78a49ce9
==========

[zra, 2016-02-08 16:41:53]

Committed patchset #13 (id:240001) manually as
2cf6405ca9fe3f87d90ed75d015c960b78a49ce9 (presubmit successful).