Add built-in root certificates to dart:io SecureSocket.

Add built-in root certificates to dart:io SecureSocket.
Add flag that allows them to disabled at runtime.

BUG=

Committed: https://code.google.com/p/dart/source/detail?r=15634

[Bill Hesse, 2012-11-29 14:17:11]

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/pkcert/pk...
File tests/standalone/io/pkcert/pkcs11.txt (left):

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/pkcert/pk...
tests/standalone/io/pkcert/pkcs11.txt:2: name=NSS Internal PKCS #11 Module
This file is automatically recreated and modified by NSS when the database is
opened, so we should not check it in to the repository.

[Mads Ager (google), 2012-11-29 20:13:05]

That was easier than expected. :-)

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:27: * then an built-in set of root certificates
for trusted certificate
an built-in set -> a built-in set?

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:36: external static void
setCertificateDatabase(String certificateDatabase,
How do you get away with only using the built-in set of root certificates when
you have to pass in the string here? How about renaming this to initialize and
making all arguments optional so that you can choose to just call initialize
with no arguments to get the default initialization with only the built-in set
of certificates?

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:306: try {
I think we should do our best to avoid this type of try-catch. It makes it
really hard to track down problems because you are eating the original stack
trace. Can you explain the need to wrap here? It seems to me that there is
nothing the user can do anyway if we are throwing errors in the internals here,
so I'm not sure why catching the exception for internal stuff is better?

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_bu...
File tests/standalone/io/secure_builtin_roots_test.dart (right):

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_bu...
tests/standalone/io/secure_builtin_roots_test.dart:40:
SecureSocket.setCertificateDatabase(testPkcertDatabase.toNativePath());
This is where it would be nice to just call SecureSocket.initialize().

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_no...
File tests/standalone/io/secure_no_builtin_roots_test.dart (right):

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_no...
tests/standalone/io/secure_no_builtin_roots_test.dart:37:
SecureSocket.setCertificateDatabase(testPkcertDatabase.toNativePath(),
SecureSocket.initialize(useBuiltinRoots: false); would be great here.

[Bill Hesse, 2012-11-30 12:51:16]

Comments addressed.  All parameters to SecureSocket.initialize are now optional.
Fixed a crash with multiple processes accessing the
database with write access.

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:27: * then an built-in set of root certificates
for trusted certificate
On 2012/11/29 20:13:05, Mads Ager wrote:
> an built-in set -> a built-in set?

Done.

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:36: external static void
setCertificateDatabase(String certificateDatabase,
On 2012/11/29 20:13:05, Mads Ager wrote:
> How do you get away with only using the built-in set of root certificates when
> you have to pass in the string here? How about renaming this to initialize and
> making all arguments optional so that you can choose to just call initialize
> with no arguments to get the default initialization with only the built-in set
> of certificates?

Done.

https://codereview.chromium.org/11308271/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:306: try {
On 2012/11/29 20:13:05, Mads Ager wrote:
> I think we should do our best to avoid this type of try-catch. It makes it
> really hard to track down problems because you are eating the original stack
> trace. Can you explain the need to wrap here? It seems to me that there is
> nothing the user can do anyway if we are throwing errors in the internals
here,
> so I'm not sure why catching the exception for internal stuff is better?

In the socket class, the onError handler can be called both asynchronously and
during a call to read, write, or available.  There are also exceptions thrown,
if any of the user callbacks throw an exception.  So this will reproduce the
current behavior, just for this function.  Other functions still are throwing
exceptions where they should call onError.

Changed the scope of the try block to avoid the rethrow.

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_bu...
File tests/standalone/io/secure_builtin_roots_test.dart (right):

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_bu...
tests/standalone/io/secure_builtin_roots_test.dart:40:
SecureSocket.setCertificateDatabase(testPkcertDatabase.toNativePath());
On 2012/11/29 20:13:05, Mads Ager wrote:
> This is where it would be nice to just call SecureSocket.initialize().

Done.

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_no...
File tests/standalone/io/secure_no_builtin_roots_test.dart (right):

https://codereview.chromium.org/11308271/diff/1/tests/standalone/io/secure_no...
tests/standalone/io/secure_no_builtin_roots_test.dart:37:
SecureSocket.setCertificateDatabase(testPkcertDatabase.toNativePath(),
On 2012/11/29 20:13:05, Mads Ager wrote:
> SecureSocket.initialize(useBuiltinRoots: false); would be great here.

Done.

https://codereview.chromium.org/11308271/diff/5001/tests/standalone/io/pkcert...
File tests/standalone/io/pkcert/pkcs11.txt (left):

https://codereview.chromium.org/11308271/diff/5001/tests/standalone/io/pkcert...
tests/standalone/io/pkcert/pkcs11.txt:2: name=NSS Internal PKCS #11 Module
This file is removed because it is created and modified by NSS when a
certificate database is opened read/write.  It varies from user to user, and
will no longer be created now that we open the database readonly.

[Mads Ager (google), 2012-12-03 09:15:08]

One question. The rest is looking good.

https://codereview.chromium.org/11308271/diff/7001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11308271/diff/7001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:266: // This will not open a database in the
current directory, even if it
This looks strange. The documentation just says that this ignores failures to
open the database. Can you provide a pointer to the documentation that this will
not open a database?

Based on the documentation I would have guessed that NSS_INIT_NOCERTDB would be
the right flag?

https://codereview.chromium.org/11308271/diff/7001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11308271/diff/7001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:19: * Initializes the NSS library with the path to
a certificate database
We should revisit this comment more. We should provide a couple of examples:

// Use only builtin db.
init(useBuiltinRoots: true);
// Use own db and merge with builtin db.
init(database: 'mydb', password: 'mypass');
// Use only own db.
init(database: 'mydb', password:'mypass', useBuiltinRoots: false);

[Bill Hesse, 2012-12-03 12:20:04]

There is a VM crash on Windows (7102), when an exception is thrown from native
code during event processing.  Filed bug and added to status file.

https://codereview.chromium.org/11308271/diff/7001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11308271/diff/7001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:266: // This will not open a database in the
current directory, even if it
On 2012/12/03 09:15:09, Mads Ager wrote:
> This looks strange. The documentation just says that this ignores failures to
> open the database. Can you provide a pointer to the documentation that this
will
> not open a database?
> 
> Based on the documentation I would have guessed that NSS_INIT_NOCERTDB would
be
> the right flag?

I was referring only to passing "" as the database path, and saying that that is
different than passing "." as the database path.  The flag has nothing to do
with it, except that it would otherwise signal an error.  The code for
NSS_Init_NoDB passes the FORCEOPEN flag, along with the others.

The NOCERTDB flag causes Windows to fail.  The documentation sounds as if it
should be the right thing to set, but in practice it does not work.

Changed the comment to be more explicit.

[Mads Ager (google), 2012-12-03 13:00:13]

LGTM

https://codereview.chromium.org/11308271/diff/7001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11308271/diff/7001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:266: // This will not open a database in the
current directory, even if it
On 2012/12/03 12:20:04, Bill Hesse wrote:
> On 2012/12/03 09:15:09, Mads Ager wrote:
> > This looks strange. The documentation just says that this ignores failures
to
> > open the database. Can you provide a pointer to the documentation that this
> will
> > not open a database?
> > 
> > Based on the documentation I would have guessed that NSS_INIT_NOCERTDB would
> be
> > the right flag?
> 
> I was referring only to passing "" as the database path, and saying that that
is
> different than passing "." as the database path.  The flag has nothing to do
> with it, except that it would otherwise signal an error.  The code for
> NSS_Init_NoDB passes the FORCEOPEN flag, along with the others.
> 
> The NOCERTDB flag causes Windows to fail.  The documentation sounds as if it
> should be the right thing to set, but in practice it does not work.
> 
> Changed the comment to be more explicit.

Thanks for updating the comment. Please add the part about NOCERTDB not working
on Windows as well. I would add it as a TODO to figure out why that does not
work. Please document the way in which Windows does not work when using NOCERTDB
as well.