dart:io | Add SecureSocket.importPrivateCertificates, that reads a PKCS#12 file.

dart:io | Add SecureSocket.importPrivateCertificates, that reads a PKCS#12 file.


Add private key with certificate to SecureSocket

BUG=
R=sgjesse@google.com, wtc@chromium.org

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

[Bill Hesse, 2013-08-06 16:47:59]

Here is the CL adding import of private keys from a PKCS#12 file to dart:io.  I
will sent it to more people, once you take a look.  We need to add the contents
of the pkcs12 directory from NSS to third_party to enable this CL, or get pkcs12
added upstream in Chromium NSS.

wtc - please take a look at secure_socket.cc to see if I am misusing the pk11
and pk12 API in any big ways.  I'm sure I'm doing at least one thing in a
non-standard way, because I don't know the right way.

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

https://codereview.chromium.org/21716004/diff/3001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:111: char* PasswordCallback(PK11SlotInfo* slot,
PRBool retry, void* arg) {
Moved here from later in the source file.

https://codereview.chromium.org/21716004/diff/3001/runtime/bin/secure_socket.h
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/21716004/diff/3001/runtime/bin/secure_socket....
runtime/bin/secure_socket.h:100: static char* password_;
Most of the uses of the password require it to be passed as a void*, so we drop
the const here rather than casting everywhere.

We could also just make this public, rather than making an accessor for use in
non-member functions.

[wtc, 2013-08-06 17:52:47]

Review comments on patch set 3:

I only reviewed the three runtime/bin/secure_socket* files.
Overall the code seems correct, but I have to admit I am not
familiar with the NSS PKCS #12 functions, so I also checked
how they are used by NSS itself.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/net/nss.gyp
File runtime/bin/net/nss.gyp (right):

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/net/nss.gyp#ne...
runtime/bin/net/nss.gyp:20: 'pkcs12_directory': '../../../third_party/pkcs12',

I recommend that you name the new pkcs12_directory "nss_pkcs12" instead of just
"pkcs12" to make it clear that it comes from NSS.

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

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:395: void *arg) {

Nit: fix the indentation of the second and third function arguments.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:401: void
FUNCTION_NAME(SecureSocket_ImportPrivateCertificates)

Certificates with corresponding private keys are called "user certificates" in
NSS. I don't think "user certificates" is standard terminology, but "private
certificates" isn't, either. Perhaps we can name this function
SecureSocket_ImportCertificatesAndPrivateKeys or
SecureSocket_ImportCertificatesWithPrivateKeys, if you think "user certificates"
is confusing?

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:442: p12_password.data = (unsigned char*)password;

Nit: reinterpret_cast<unsigned char*>(password)

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:475: } else {

Nit: some projects have a convention of omitting "else" after a return
statement.
I am not sure if Dart follows this convention.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:510: ThrowPRException("CertificateException",
"Trust string cannot be decoded");

If these ThrowPRException calls also return from the function, you need to
make sure CERT_DestroyCertificate(certificate) is called.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:549: }

CERT_DestroyCertificate(certificate) should also be called when
the certificate in the database is successfully deleted. So this
if statement should be rewritten as:

  CERT_DestroyCertificate(certificate);
  if (status == SECFailure) {
    ThrowCertificateException("Cannot remove certificate %s", nickname);
  }

Nit: in general, it is better to test != SECSuccess than == SECFailure
because SECSuccess is 0. This is old school C programmer's optimization :-)

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:845: // Allow encoding and decoding of private keys
in PKCS#12 files (.pk files).

Common file suffixes for PKCS #11 files are .p12 and .pfx (used by Microsoft).
Some Linux apps use .pkcs12. I have not seen the .pk file suffix before.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:852:
SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);

I know these function calls are modeled after the NSS pk12util tool.
But many of these ciphers are weak or old. I suggest you start with just

    SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
    SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);

(PKCS12_DES_EDE3_168 is Triple DES.)

This will help us discover whether any of the weak or old cipher suites
is actually needed for Dart.

Also, move this block of code before the NSS_SetDomesticPolicy() call.
NSS_SetDomesticPolicy is actually a libSSL function, so it is better for
it to stay with the other SSL_xxx calls.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:957: SECKEYPrivateKey* key =
PK11_FindKeyByAnyCert(certificate, GetPassword());

You don't need to pass the password as the "void *wincx" argument to
PK11_FindCertFromNickname and PK11_FindKeyByAnyCert.  "wincx" stands for
"window context", and is supposed to be the window handle for the password
dialog. You can pass NULL as the "void *wincx" argument.

Your 'PasswordCallback' function can just duplicate the static data member
SSLFilter::password_ and return the copy.

Of course, the current method also works.

[Søren Gjesse, 2013-08-07 07:32:28]

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

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:110: 
Maybe move this function closer to where it is used.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:418: uint8_t* pk12 = Dart_ScopeAllocate(length +
1);  // Why +1 ?
Yes, why + 1?

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:424: // A big-endian Unicode (UTF16) password.
So the PKCS12 functions use a password in big-endian UTF16 stored in a SECItem
with type siBuffer? It is not possible to use type siUTF8String which seems more
logical? Probably not.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:424: // A big-endian Unicode (UTF16) password.
Also you could consider building this in Dart code, but of cause now it is
already in C++.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:446: // Set the password callback for the
certificate database we are importing to.
Please explain how this callback is used by nss. It seems to just return one of
its arguments.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:472: SEC_PKCS12DecoderFinish(context);
"Hiding" the Dart_ExitScope here for the succesful return seems a bit dangerous.
How about:

if (SECSuccess != SEC_PKC... ||
    SECSuccess != SEC_PKC... ||
    ... ) {
  SEC_PKCS12DecoderFinish(context);
  // throw
}
SEC_PKCS12DecoderFinish(context);
Dart_ExitScope();

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:479: }
Nice use of Dart_ScopeAllocate - Only the context to "free" before returning.

The "slot" does not need "freeing/releasing"?

How about the PK11_SetPasswordFunc - should that be reset?

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

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:244: * their private keys to the in-memory
certificate database.
As I understood it is not only imported into the in-memory certificate database
but also stored persistently in it. It is great if it is only transient in the
in-memory database.

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:248: * .pk files.
Change the .pk extension.

Put `` around extensions.

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:249: *
If limiting the ciphers please list the range which can actually be used here.

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:251: * uses of each certificate must be added with
SecureSocket.changeTrust.
Add `` around SecureSocket.changeTrust.

https://codereview.chromium.org/21716004/diff/6001/tests/standalone/io/privat...
File tests/standalone/io/private_certificate_test.dart (right):

https://codereview.chromium.org/21716004/diff/6001/tests/standalone/io/privat...
tests/standalone/io/private_certificate_test.dart:70: .then((server) =>
server..listen(
Why ..?

[Bill Hesse, 2013-08-08 17:39:20]

OK, PTAL.  I still would like feedback on whether to add the <10 pkcs12 NSS
source files to runtime/third_party/nss_pkcs12, to third_party/nss_pkcs12, or to
upstream Chromium NSS.

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

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:110: 
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Maybe move this function closer to where it is used.

I have had to keep moving it up and up, as I add uses of it.
And it is used in multiple places.  But I will move it to before its first use.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:395: void *arg) {
On 2013/08/06 17:52:48, wtc wrote:
> 
> Nit: fix the indentation of the second and third function arguments.

Done.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:401: void
FUNCTION_NAME(SecureSocket_ImportPrivateCertificates)
On 2013/08/06 17:52:48, wtc wrote:
> 
> Certificates with corresponding private keys are called "user certificates" in
> NSS. I don't think "user certificates" is standard terminology, but "private
> certificates" isn't, either. Perhaps we can name this function
> SecureSocket_ImportCertificatesAndPrivateKeys or
> SecureSocket_ImportCertificatesWithPrivateKeys, if you think "user
certificates"
> is confusing?

Done.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:424: // A big-endian Unicode (UTF16) password.
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Also you could consider building this in Dart code, but of cause now it is
> already in C++.

The behavior depends on whether the host is big-endian or not, and we already
have that in Utils::HostToBigEndian16.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:442: p12_password.data = (unsigned char*)password;
On 2013/08/06 17:52:48, wtc wrote:
> 
> Nit: reinterpret_cast<unsigned char*>(password)

Done.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:446: // Set the password callback for the
certificate database we are importing to.
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Please explain how this callback is used by nss. It seems to just return one
of
> its arguments.
 It returns a newly allocated copy of its argument.

This is built into NSS, that the password for a slot is gotten from a callback,
and it is freed by the callee.
It could always return the password, without using the arg, but this ensures
that the caller of the password callback has been given the password, and is
thus authorized.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:472: SEC_PKCS12DecoderFinish(context);
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> "Hiding" the Dart_ExitScope here for the succesful return seems a bit
dangerous.
> How about:
> 
> if (SECSuccess != SEC_PKC... ||
>     SECSuccess != SEC_PKC... ||
>     ... ) {
>   SEC_PKCS12DecoderFinish(context);
>   // throw
> }
> SEC_PKCS12DecoderFinish(context);
> Dart_ExitScope();

No more EnterScope and ExitScopes.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:475: } else {
On 2013/08/06 17:52:48, wtc wrote:
> 
> Nit: some projects have a convention of omitting "else" after a return
> statement.
> I am not sure if Dart follows this convention.

Code rewritten differently.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:479: }
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Nice use of Dart_ScopeAllocate - Only the context to "free" before returning.
> 
> The "slot" does not need "freeing/releasing"?
> 
> How about the PK11_SetPasswordFunc - should that be reset?

The password callback stays the same for the entire process.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:510: ThrowPRException("CertificateException",
"Trust string cannot be decoded");
On 2013/08/06 17:52:48, wtc wrote:
> 
> If these ThrowPRException calls also return from the function, you need to
> make sure CERT_DestroyCertificate(certificate) is called.

Done.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:549: }
On 2013/08/06 17:52:48, wtc wrote:
> 
> CERT_DestroyCertificate(certificate) should also be called when
> the certificate in the database is successfully deleted. So this
> if statement should be rewritten as:
> 
>   CERT_DestroyCertificate(certificate);
>   if (status == SECFailure) {
>     ThrowCertificateException("Cannot remove certificate %s", nickname);
>   }
> 
> Nit: in general, it is better to test != SECSuccess than == SECFailure
> because SECSuccess is 0. This is old school C programmer's optimization :-)

Done.

https://codereview.chromium.org/21716004/diff/6001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:852:
SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);
On 2013/08/06 17:52:48, wtc wrote:
> 
> I know these function calls are modeled after the NSS pk12util tool.
> But many of these ciphers are weak or old. I suggest you start with just
> 
>     SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
>     SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);
> 
> (PKCS12_DES_EDE3_168 is Triple DES.)
> 
> This will help us discover whether any of the weak or old cipher suites
> is actually needed for Dart.
> 
> Also, move this block of code before the NSS_SetDomesticPolicy() call.
> NSS_SetDomesticPolicy is actually a libSSL function, so it is better for
> it to stay with the other SSL_xxx calls.

Done.

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

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:244: * their private keys to the in-memory
certificate database.
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> As I understood it is not only imported into the in-memory certificate
database
> but also stored persistently in it. It is great if it is only transient in the
> in-memory database.

Removed in-memory database.  Will check what happens in the various cases.

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:248: * .pk files.
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Change the .pk extension.
> 
> Put `` around extensions.

Done.

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:249: *
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> If limiting the ciphers please list the range which can actually be used here.

Done.

https://codereview.chromium.org/21716004/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:251: * uses of each certificate must be added with
SecureSocket.changeTrust.
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Add `` around SecureSocket.changeTrust.

Done.

https://codereview.chromium.org/21716004/diff/6001/tests/standalone/io/privat...
File tests/standalone/io/private_certificate_test.dart (right):

https://codereview.chromium.org/21716004/diff/6001/tests/standalone/io/privat...
tests/standalone/io/private_certificate_test.dart:70: .then((server) =>
server..listen(
On 2013/08/07 07:32:28, Søren Gjesse wrote:
> Why ..?

We want to return the SecureServerSocket, not the return value from listen, a
subscription.

[Søren Gjesse, 2013-08-08 19:03:42]

You can place the PKCS12 files in the third_party in the root of the SVN
repository for now, and then add a reference to it in all.deps/DEPS. If you get
it in to Chromium NSS we can start pulling it from there instead.

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

https://codereview.chromium.org/21716004/diff/22001/sdk/lib/io/secure_socket....
sdk/lib/io/secure_socket.dart:253: * readOnly set to false.
`` around false.

https://codereview.chromium.org/21716004/diff/22001/tests/standalone/io/certi...
File tests/standalone/io/certificate_test_client.dart (right):

https://codereview.chromium.org/21716004/diff/22001/tests/standalone/io/certi...
tests/standalone/io/certificate_test_client.dart:16: // 
SecureSocket.initialize(database:
"/usr/local/google/home/whesse/ssd/dart/tests/standalone/io/pkcert", password:
'dartdart', readOnly: false);
Long line and absolute path to your home dir.

[Søren Gjesse, 2013-08-08 19:03:59]

LGTM

[Ivan Posva, 2013-08-08 19:38:17]

It really makes me uncomfortable to be able to add/register certificates for the
whole process. This breaks per-isolate security and we should discuss the impact
outside of a code review of this change in behaviour.

-Ivan

[wtc, 2013-08-08 20:01:13]

Patch set 6 LGTM. I only reviewed the three
runtime/bin/secure_socket* files.

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

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:350: PK11_SetPasswordFunc(PasswordCallback);

We should ideally call PK11_SetPasswordFunc just once, when
we initialize NSS. Is it really necessary to call
PK11_SetPasswordFunc here?

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:357: PK11_FreeSlot(slot);

Free |slot| before lie 353.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:408: uint8_t* pk12 = Dart_ScopeAllocate(length +
1);  // Why +1 ?

Can you try removing "+ 1"?

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:419: Dart_ScopeAllocate(2 * password_length));

Nit: replace 2 with sizeof(uint16_t). You may ignore this.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:433: p12_password.len = 2 * password_length;

Nit: same here: replace 2 with sizeof(uint16_t).

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:850: SEC_PKCS12EnableCipher(PKCS12_RC2_CBC_40, 1);

I assume you found PKCS12_RC2_CBC_40 is required. It is rather
weak.

[Bill Hesse, 2013-08-09 13:24:05]

Added some locking around calls that mutate the database.  NSS does a very good
job of being multithreaded already, it seems, so these might not even be
necessary.

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

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:350: PK11_SetPasswordFunc(PasswordCallback);
On 2013/08/08 20:01:13, wtc wrote:
> 
> We should ideally call PK11_SetPasswordFunc just once, when
> we initialize NSS. Is it really necessary to call
> PK11_SetPasswordFunc here?

No, it isn't.  Removed.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:357: PK11_FreeSlot(slot);
On 2013/08/08 20:01:13, wtc wrote:
> 
> Free |slot| before lie 353.

Done.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:408: uint8_t* pk12 = Dart_ScopeAllocate(length +
1);  // Why +1 ?
On 2013/08/08 20:01:13, wtc wrote:
> 
> Can you try removing "+ 1"?

Done.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:419: Dart_ScopeAllocate(2 * password_length));
On 2013/08/08 20:01:13, wtc wrote:
> 
> Nit: replace 2 with sizeof(uint16_t). You may ignore this.

Done.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:433: p12_password.len = 2 * password_length;
On 2013/08/08 20:01:13, wtc wrote:
> 
> Nit: same here: replace 2 with sizeof(uint16_t).

Done.

https://codereview.chromium.org/21716004/diff/22001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:850: SEC_PKCS12EnableCipher(PKCS12_RC2_CBC_40, 1);
On 2013/08/08 20:01:13, wtc wrote:
> 
> I assume you found PKCS12_RC2_CBC_40 is required. It is rather
> weak.

Yes.  The certificates are encrypted with RC2, the keys with triple DES.

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

https://codereview.chromium.org/21716004/diff/22001/sdk/lib/io/secure_socket....
sdk/lib/io/secure_socket.dart:253: * readOnly set to false.
On 2013/08/08 19:03:42, Søren Gjesse wrote:
> `` around false.

Done.

[Bill Hesse, 2013-08-12 12:06:59]

Added "delete_a_directory_later.dart" and detatched call to it in
"user_certificate_test.dart".

[Søren Gjesse, 2013-08-12 12:37:56]

lgtm

https://codereview.chromium.org/21716004/diff/51001/tests/standalone/io/user_...
File tests/standalone/io/user_certificate_test.dart (right):

https://codereview.chromium.org/21716004/diff/51001/tests/standalone/io/user_...
tests/standalone/io/user_certificate_test.dart:33: // The certificate database
files are locked until then.
I think we should open a bug on improving this. This could lead to issues with
building right after running tests as dart.exe will be locked for a period.

Using the dart.exe used for the testrunner could fix that.

[Bill Hesse, 2013-08-12 13:51:04]

Committed patchset #9 manually as r26002 (presubmit successful).

[Bill Hesse, 2013-08-19 10:30:13]

On 2013/08/12 13:51:04, Bill Hesse wrote:
> Committed patchset #9 manually as r26002 (presubmit successful).

This patchset was reverted in r26194 (https://codereview.chromium.org/22887014/)

Because it affects all isolates, and its functionality can be accomplished
through shell commands,
it was reverted.  If we commit it again, we should put checks on the C++ uses of
certificate length to check against overflow:

  "It would probably make sense to add some sanity checking against the
"password" string object's length in the
SecureSocket_ImportCertificatesWithPrivateKeys function. There are multiple
arithmetic operations being performed there (+1, *sizeof(uint16_t), etc) with
different variable types ("int" in SECItem, "intptr_t" in your code) and I
imagine it would be potentially possible to create a 2GB+ string in 64-bit
Chrome process."